From f480086b6bf8f78341324da7a165d18b10386674 Mon Sep 17 00:00:00 2001 From: Julien Laurenceau Date: Mon, 12 Oct 2026 16:11:06 +0200 Subject: [PATCH 01/14] fix: return 503 instead of 404 when commit metadata is missing When the staged xl.meta disappears between the data rename and the meta-file commit inside RenameData(), the affected drives return errFileNotFound. An in-quorum errFileNotFound is reduced by reduceWriteQuorumErrs() and previously surfaced as: - IncompleteBody (400) from PutObject(), misattributing the failure to a client disconnect, or - ObjectNotFound (404 NoSuchKey) from CompleteMultipartUpload(), telling clients the object is absent so they skip retries. Map errFileNotFound at the commit step to a dedicated internal errCommitMetaMissing, convert it to errErasureWriteQuorum in renameData(), and map it to InsufficientWriteQuorum (HTTP 503) in toObjectErr() so clients retry. Below-quorum outcomes already return InsufficientWriteQuorum and are unchanged, as is the IncompleteBody mapping for staging failures of client-disconnected uploads. Signed-off-by: Julien Laurenceau --- cmd/erasure-metadata-utils_test.go | 30 ++++++++++++++++++++++++++++++ cmd/erasure-object.go | 6 ++++++ cmd/object-api-errors.go | 2 +- cmd/storage-errors.go | 5 +++++ cmd/storage-rest-client.go | 2 ++ cmd/xl-storage-errors_test.go | 26 ++++++++++++++++++++++++++ cmd/xl-storage.go | 10 +++++++++- 7 files changed, 79 insertions(+), 2 deletions(-) diff --git a/cmd/erasure-metadata-utils_test.go b/cmd/erasure-metadata-utils_test.go index b14cc25df..8fc1b4be9 100644 --- a/cmd/erasure-metadata-utils_test.go +++ b/cmd/erasure-metadata-utils_test.go @@ -20,6 +20,7 @@ package cmd import ( "context" "encoding/hex" + "errors" "fmt" "math/rand" "reflect" @@ -110,6 +111,35 @@ func TestReduceErrs(t *testing.T) { } } +// TestReduceWriteQuorumErrsCommitMetaMissing verifies that an in-quorum +// errCommitMetaMissing reduces to the sentinel, which renameData() converts +// to errErasureWriteQuorum, while below-quorum reduces directly to +// errErasureWriteQuorum. +func TestReduceWriteQuorumErrsCommitMetaMissing(t *testing.T) { + errs := make([]error, 16) + for i := range 9 { + errs[i] = errCommitMetaMissing + } + if err := reduceWriteQuorumErrs(t.Context(), errs, objectOpIgnoredErrs, 9); !errors.Is(err, errCommitMetaMissing) { + t.Errorf("expected %s, got %s", errCommitMetaMissing, err) + } + // The sentinel must be distinguishable from errFileNotFound so that + // commit failures are never mapped to ObjectNotFound (404). + if errors.Is(errCommitMetaMissing, errFileNotFound) { + t.Error("errCommitMetaMissing must not match errFileNotFound") + } + + // Below quorum the reduction must yield a write-quorum failure (503); + // eight sentinels against eight committed drives is not a quorum. + belowQuorum := make([]error, 16) + for i := range 8 { + belowQuorum[i] = errCommitMetaMissing + } + if err := reduceWriteQuorumErrs(t.Context(), belowQuorum, objectOpIgnoredErrs, 9); !errors.Is(err, errErasureWriteQuorum) { + t.Errorf("expected %s, got %s", errErasureWriteQuorum, err) + } +} + // TestHashOrder - test order of ints in array func TestHashOrder(t *testing.T) { testCases := []struct { diff --git a/cmd/erasure-object.go b/cmd/erasure-object.go index 8ab3d17a1..4bf060440 100644 --- a/cmd/erasure-object.go +++ b/cmd/erasure-object.go @@ -1072,6 +1072,12 @@ func renameData(ctx context.Context, disks []StorageAPI, srcBucket, srcEntry str errs := g.Wait() err := reduceWriteQuorumErrs(ctx, errs, objectOpIgnoredErrs, writeQuorum) + if errors.Is(err, errCommitMetaMissing) { + // Commit metadata went missing on a quorum of drives; the write + // did not commit. Return a 503 class error so clients retry + // instead of treating the object as absent (404). + err = errErasureWriteQuorum + } if err != nil { dg := errgroup.WithNErrs(len(disks)) for index, nerr := range errs { diff --git a/cmd/object-api-errors.go b/cmd/object-api-errors.go index 7b2b5e404..457970fb0 100644 --- a/cmd/object-api-errors.go +++ b/cmd/object-api-errors.go @@ -162,7 +162,7 @@ func toObjectErr(oerr error, params ...string) error { apiErr.Type = v.Type } return apiErr - case errErasureWriteQuorum.Error(): + case errErasureWriteQuorum.Error(), errCommitMetaMissing.Error(): apiErr := InsufficientWriteQuorum{} if len(params) >= 1 { apiErr.Bucket = params[0] diff --git a/cmd/storage-errors.go b/cmd/storage-errors.go index 69ffcebb0..c0a908eac 100644 --- a/cmd/storage-errors.go +++ b/cmd/storage-errors.go @@ -70,6 +70,11 @@ var errDiskAccessDenied = StorageErr("drive access denied") // errFileNotFound - cannot find the file. var errFileNotFound = StorageErr("file not found") +// errCommitMetaMissing - the staged xl.meta disappeared between the +// data rename and the meta-file commit; a write-quorum failure, not +// a missing object. +var errCommitMetaMissing = StorageErr("commit metadata is missing") + // errFileNotFound - cannot find requested file version. var errFileVersionNotFound = StorageErr("file version not found") diff --git a/cmd/storage-rest-client.go b/cmd/storage-rest-client.go index 76993bc06..0ba573847 100644 --- a/cmd/storage-rest-client.go +++ b/cmd/storage-rest-client.go @@ -106,6 +106,8 @@ func toStorageErr(err error) error { return errVolumeExists case errFileNotFound.Error(): return errFileNotFound + case errCommitMetaMissing.Error(): + return errCommitMetaMissing case errFileVersionNotFound.Error(): return errFileVersionNotFound case errFileNameTooLong.Error(): diff --git a/cmd/xl-storage-errors_test.go b/cmd/xl-storage-errors_test.go index 0a4757765..0551fed6a 100644 --- a/cmd/xl-storage-errors_test.go +++ b/cmd/xl-storage-errors_test.go @@ -18,6 +18,7 @@ package cmd import ( + "errors" "os" "runtime" "syscall" @@ -56,3 +57,28 @@ func TestSysErrors(t *testing.T) { } } } + +// TestCommitMetaMissingMapping verifies the RPC and object-layer mapping +// of errCommitMetaMissing, returned when the staged xl.meta disappears +// between the data rename and the meta-file commit in RenameData(). +func TestCommitMetaMissingMapping(t *testing.T) { + // The sentinel must survive a storage REST/grid round-trip; errors + // cross the wire as strings and are remapped by toStorageErr(). + if got := toStorageErr(errors.New(errCommitMetaMissing.Error())); !errors.Is(got, errCommitMetaMissing) { + t.Fatalf("expected %s, got %s", errCommitMetaMissing, got) + } + if got := toStorageErr(errCommitMetaMissing); got != errCommitMetaMissing { + t.Fatalf("expected %s, got %s", errCommitMetaMissing, got) + } + + // toObjectErr() must map the sentinel to InsufficientWriteQuorum + // (503), never to ObjectNotFound (404). + err := toObjectErr(errCommitMetaMissing, "bucket", "object") + qerr, ok := err.(InsufficientWriteQuorum) + if !ok { + t.Fatalf("expected InsufficientWriteQuorum, got %s (%T)", err, err) + } + if qerr.Bucket != "bucket" || qerr.Object != "object" { + t.Fatalf("unexpected bucket/object %s/%s", qerr.Bucket, qerr.Object) + } +} diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index 92d8c8dec..81e415553 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -2925,7 +2925,15 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, f // if its a partial rename() do not attempt to delete recursively. // this can be healed since all parts are available. s.deleteFile(dstVolumeDir, dstDataPath, false, false) - return res, osErrToFileErr(err) + err = osErrToFileErr(err) + if errors.Is(err, errFileNotFound) { + // The staged xl.meta disappeared between staging and commit. + // This is a write-quorum failure, not a missing object; use a + // dedicated error so the object layer returns 503 instead of + // mapping it to ObjectNotFound (404). + err = errCommitMetaMissing + } + return res, err } if srcVolume != minioMetaMultipartBucket { From 00d4a206f75a6ae5b51bf60e8bcadb2b33662c6d Mon Sep 17 00:00:00 2001 From: Julien Laurenceau Date: Mon, 12 Oct 2026 17:02:02 +0200 Subject: [PATCH 02/14] fix: zero EcM when unmarshaling legacy xl.meta V2 headers The legacy header decode paths zeroed EcN twice (z.EcN, z.EcN = 0, 0) instead of EcN and EcM, so EcM retained its previous value when the same structure was reused across decodes. Decoding an xl.meta written by an older release would then leak the erasure metadata (data/parity block counts) of a previously decoded object into it, potentially corrupting quorum and healing decisions. Also zero the fields on the V1 header path, which had no reset at all. Signed-off-by: Julien Laurenceau --- cmd/xl-storage-format-v2-legacy.go | 5 ++- cmd/xl-storage-format-v2_test.go | 66 ++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/cmd/xl-storage-format-v2-legacy.go b/cmd/xl-storage-format-v2-legacy.go index 9d682487b..188b299d3 100644 --- a/cmd/xl-storage-format-v2-legacy.go +++ b/cmd/xl-storage-format-v2-legacy.go @@ -40,6 +40,7 @@ func (x *xlMetaV2VersionHeader) unmarshalV(v uint8, bts []byte) (o []byte, err e // unmarshalV1 decodes version 1, never released. func (x *xlMetaV2VersionHeader) unmarshalV1(bts []byte) (o []byte, err error) { + x.EcN, x.EcM = 0, 0 var zb0001 uint32 zb0001, bts, err = msgp.ReadArrayHeaderBytes(bts) if err != nil { @@ -134,7 +135,7 @@ type xlMetaV2VersionHeaderV2 struct { // UnmarshalMsg implements msgp.Unmarshaler func (z *xlMetaV2VersionHeaderV2) UnmarshalMsg(bts []byte) (o []byte, err error) { - z.EcN, z.EcN = 0, 0 + z.EcN, z.EcM = 0, 0 var zb0001 uint32 zb0001, bts, err = msgp.ReadArrayHeaderBytes(bts) if err != nil { @@ -184,7 +185,7 @@ func (z *xlMetaV2VersionHeaderV2) UnmarshalMsg(bts []byte) (o []byte, err error) // DecodeMsg implements msgp.Decodable func (z *xlMetaV2VersionHeaderV2) DecodeMsg(dc *msgp.Reader) (err error) { - z.EcN, z.EcN = 0, 0 + z.EcN, z.EcM = 0, 0 var zb0001 uint32 zb0001, err = dc.ReadArrayHeader() if err != nil { diff --git a/cmd/xl-storage-format-v2_test.go b/cmd/xl-storage-format-v2_test.go index 6a9b15a3f..21e76eb76 100644 --- a/cmd/xl-storage-format-v2_test.go +++ b/cmd/xl-storage-format-v2_test.go @@ -38,6 +38,7 @@ import ( "github.com/klauspost/compress/zstd" "github.com/minio/minio/internal/bucket/lifecycle" xhttp "github.com/minio/minio/internal/http" + "github.com/tinylib/msgp/msgp" ) func TestReadXLMetaNoData(t *testing.T) { @@ -1190,3 +1191,68 @@ func BenchmarkToFileInfoNoParts(b *testing.B) { func BenchmarkToFileInfoWithParts(b *testing.B) { benchmarkManyPartsOptionally(b, true) } + +// TestXLMetaV2HeaderLegacyUnmarshalResetsEc checks that decoding legacy +// version headers, which carry no erasure metadata fields, cannot leak +// stale EcN/EcM values from a previously decoded object into the same +// structure. +func TestXLMetaV2HeaderLegacyUnmarshalResetsEc(t *testing.T) { + base := xlMetaV2VersionHeader{ + VersionID: [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, + ModTime: time.Now().Unix(), + Signature: [4]byte{1, 2, 3, 4}, + Type: ObjectType, + Flags: xlFlagInlineData, + } + + // Legacy V2 header wire format: 5 fields, no EcN/EcM. + var legacy []byte + legacy = msgp.AppendArrayHeader(legacy, 5) + legacy = msgp.AppendBytes(legacy, base.VersionID[:]) + legacy = msgp.AppendInt64(legacy, base.ModTime) + legacy = msgp.AppendBytes(legacy, base.Signature[:]) + legacy = msgp.AppendUint8(legacy, uint8(base.Type)) + legacy = msgp.AppendUint8(legacy, uint8(base.Flags)) + + header := base + header.EcN, header.EcM = 4, 8 + x2 := xlMetaV2VersionHeaderV2{xlMetaV2VersionHeader: &header} + if _, err := x2.UnmarshalMsg(legacy); err != nil { + t.Fatalf("legacy V2 header unmarshal failed: %v", err) + } + if header.EcN != 0 || header.EcM != 0 { + t.Fatalf("stale erasure metadata leaked across objects: EcN %d, EcM %d", header.EcN, header.EcM) + } + + // V1 headers (4 fields, never released) must zero the fields too. + var v1 []byte + v1 = msgp.AppendArrayHeader(v1, 4) + v1 = msgp.AppendBytes(v1, base.VersionID[:]) + v1 = msgp.AppendInt64(v1, base.ModTime) + v1 = msgp.AppendUint8(v1, uint8(base.Type)) + v1 = msgp.AppendUint8(v1, uint8(base.Flags)) + + header = base + header.EcN, header.EcM = 4, 8 + if _, err := header.unmarshalV1(v1); err != nil { + t.Fatalf("legacy V1 header unmarshal failed: %v", err) + } + if header.EcN != 0 || header.EcM != 0 { + t.Fatalf("stale erasure metadata leaked on V1 decode: EcN %d, EcM %d", header.EcN, header.EcM) + } + + // Current headers must keep their erasure metadata. + current := base + current.EcN, current.EcM = 4, 8 + wire, err := current.MarshalMsg(nil) + if err != nil { + t.Fatalf("header marshal failed: %v", err) + } + decoded := base + if _, err := decoded.UnmarshalMsg(wire); err != nil { + t.Fatalf("header unmarshal failed: %v", err) + } + if decoded.EcN != 4 || decoded.EcM != 8 { + t.Fatalf("erasure metadata lost on current decode: EcN %d, EcM %d", decoded.EcN, decoded.EcM) + } +} From a54d0c43545c5707e3dd09d9b10c4614511a423a Mon Sep 17 00:00:00 2001 From: Julien Laurenceau Date: Mon, 12 Oct 2026 17:02:02 +0200 Subject: [PATCH 03/14] fix: reset pooled NSScanner RPC responses to avoid stale usage data NSScanner responses stream either a dataUsageEntry update or the final dataUsageCache. The grid framework recycles stream responses into a pool without resetting them, and the handler set only one field per response - so a recycled response still carried the Final (or Update) payload of an earlier scan. A remote node reading such a response accepted the stale final cache, corrupting bucket usage data across scanner cycles. Honor Recycler types when pooling stream responses, implement it for nsScannerResp, and clear the unset field at the response construction sites for defense in depth. Signed-off-by: Julien Laurenceau --- cmd/storage-rest-common.go | 10 +++++++++ cmd/storage-rest-server.go | 4 ++++ cmd/storage-rest-server_test.go | 36 +++++++++++++++++++++++++++++++++ internal/grid/handlers.go | 5 +++++ 4 files changed, 55 insertions(+) create mode 100644 cmd/storage-rest-server_test.go diff --git a/cmd/storage-rest-common.go b/cmd/storage-rest-common.go index d8a28ae29..17bcc5f69 100644 --- a/cmd/storage-rest-common.go +++ b/cmd/storage-rest-common.go @@ -83,3 +83,13 @@ type nsScannerResp struct { Update *dataUsageEntry `msg:"u"` Final *dataUsageCache `msg:"f"` } + +// Recycle resets the response, so that recycled responses cannot +// leak stale scanner data into a subsequent scan. +func (r *nsScannerResp) Recycle() { + if r == nil { + return + } + r.Update = nil + r.Final = nil +} diff --git a/cmd/storage-rest-server.go b/cmd/storage-rest-server.go index f20e4e99f..17d274bca 100644 --- a/cmd/storage-rest-server.go +++ b/cmd/storage-rest-server.go @@ -290,6 +290,9 @@ func (s *storageRESTServer) NSScannerHandler(ctx context.Context, params *nsScan for update := range updates { resp := storageNSScannerRPC.NewResponse() resp.Update = &update + // Pooled responses may carry fields from a previous + // scan, always clear the fields not being set. + resp.Final = nil out <- resp } }() @@ -301,6 +304,7 @@ func (s *storageRESTServer) NSScannerHandler(ctx context.Context, params *nsScan // Send final response. resp := storageNSScannerRPC.NewResponse() resp.Final = &ui + resp.Update = nil out <- resp return nil } diff --git a/cmd/storage-rest-server_test.go b/cmd/storage-rest-server_test.go new file mode 100644 index 000000000..354348882 --- /dev/null +++ b/cmd/storage-rest-server_test.go @@ -0,0 +1,36 @@ +// Copyright (c) 2015-2023 MinIO, Inc. +// +// This file is part of MinIO Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import "testing" + +// TestNSScannerRespPooling verifies that recycled NSScanner stream +// responses cannot leak stale fields from a previous scan into new +// responses, which would otherwise produce incorrect bucket usage +// data. +func TestNSScannerRespPooling(t *testing.T) { + dirty := storageNSScannerRPC.NewResponse() + dirty.Update = &dataUsageEntry{} + dirty.Final = &dataUsageCache{} + storageNSScannerRPC.PutResponse(dirty) + + resp := storageNSScannerRPC.NewResponse() + if resp.Update != nil || resp.Final != nil { + t.Fatalf("pooled nsScannerResp leaked stale fields: Update %v, Final %v", resp.Update, resp.Final) + } +} diff --git a/internal/grid/handlers.go b/internal/grid/handlers.go index 1d20fa9b9..b0ca9e324 100644 --- a/internal/grid/handlers.go +++ b/internal/grid/handlers.go @@ -698,6 +698,11 @@ func (h *StreamTypeHandler[Payload, Req, Resp]) PutRequest(r Req) { // These should be returned by the caller. func (h *StreamTypeHandler[Payload, Req, Resp]) PutResponse(r Resp) { if r != h.nilResp { + if rc, ok := any(r).(Recycler); ok { + // Recycled stream responses must not leak fields + // from a previous use into the next response. + rc.Recycle() + } //nolint:staticcheck // SA6002 IT IS A GENERIC VALUE! (and always a pointer) h.respPool.Put(r) } From ba6e8c8d6912ba3d7f8a147e1b0710e1d1c26344 Mon Sep 17 00:00:00 2001 From: Julien Laurenceau Date: Mon, 12 Oct 2026 17:12:49 +0200 Subject: [PATCH 04/14] fix: reset Signature when unmarshaling legacy V1 xl.meta headers The V1 header wire format has no Signature field, so decoding into a reused header leaked the previous object's signature. Reset it along with EcN/EcM so a decode fully replaces the header contents. Signed-off-by: Julien Laurenceau --- cmd/xl-storage-format-v2-legacy.go | 3 +++ cmd/xl-storage-format-v2_test.go | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/cmd/xl-storage-format-v2-legacy.go b/cmd/xl-storage-format-v2-legacy.go index 188b299d3..06201693b 100644 --- a/cmd/xl-storage-format-v2-legacy.go +++ b/cmd/xl-storage-format-v2-legacy.go @@ -40,6 +40,9 @@ func (x *xlMetaV2VersionHeader) unmarshalV(v uint8, bts []byte) (o []byte, err e // unmarshalV1 decodes version 1, never released. func (x *xlMetaV2VersionHeader) unmarshalV1(bts []byte) (o []byte, err error) { + // Fields absent from the V1 wire format must be reset since the + // header may be reused across decodes. + x.Signature = [4]byte{} x.EcN, x.EcM = 0, 0 var zb0001 uint32 zb0001, bts, err = msgp.ReadArrayHeaderBytes(bts) diff --git a/cmd/xl-storage-format-v2_test.go b/cmd/xl-storage-format-v2_test.go index 21e76eb76..0076c8046 100644 --- a/cmd/xl-storage-format-v2_test.go +++ b/cmd/xl-storage-format-v2_test.go @@ -1234,12 +1234,16 @@ func TestXLMetaV2HeaderLegacyUnmarshalResetsEc(t *testing.T) { header = base header.EcN, header.EcM = 4, 8 + header.Signature = [4]byte{9, 9, 9, 9} if _, err := header.unmarshalV1(v1); err != nil { t.Fatalf("legacy V1 header unmarshal failed: %v", err) } if header.EcN != 0 || header.EcM != 0 { t.Fatalf("stale erasure metadata leaked on V1 decode: EcN %d, EcM %d", header.EcN, header.EcM) } + if header.Signature != ([4]byte{}) { + t.Fatalf("stale signature leaked on V1 decode: %v", header.Signature) + } // Current headers must keep their erasure metadata. current := base From cb85461c420d87107a63c469a7fcc1400cedd774 Mon Sep 17 00:00:00 2001 From: Julien Laurenceau Date: Mon, 12 Oct 2026 17:24:40 +0200 Subject: [PATCH 05/14] test: cover renameData commit-metadata-missing conversion Drive renameData() with stub disks to assert that an in-quorum errCommitMetaMissing is converted to errErasureWriteQuorum (HTTP 503) and that an in-quorum errFileNotFound still surfaces unchanged so PutObject() keeps returning IncompleteBody for client disconnects. Signed-off-by: Julien Laurenceau --- cmd/erasure-metadata-utils_test.go | 43 ++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/cmd/erasure-metadata-utils_test.go b/cmd/erasure-metadata-utils_test.go index 8fc1b4be9..917d8249b 100644 --- a/cmd/erasure-metadata-utils_test.go +++ b/cmd/erasure-metadata-utils_test.go @@ -140,6 +140,49 @@ func TestReduceWriteQuorumErrsCommitMetaMissing(t *testing.T) { } } +// renameDataFailingDisk is a StorageAPI stub returning a fixed error +// from RenameData, enough to drive renameData() in unit tests. +type renameDataFailingDisk struct { + StorageAPI + err error +} + +func (d *renameDataFailingDisk) RenameData(context.Context, string, string, FileInfo, string, string, RenameOptions) (RenameDataResp, error) { + return RenameDataResp{}, d.err +} + +func (d *renameDataFailingDisk) DeleteVersion(context.Context, string, string, FileInfo, bool, DeleteOptions) error { + return nil +} + +// TestRenameDataCommitMetaMissing verifies that renameData() converts an +// in-quorum errCommitMetaMissing into errErasureWriteQuorum (HTTP 503), +// while an in-quorum errFileNotFound keeps its meaning so that +// PutObject() still reports IncompleteBody for client disconnects. +func TestRenameDataCommitMetaMissing(t *testing.T) { + metadata := make([]FileInfo, 16) + for i := range metadata { + metadata[i] = FileInfo{Deleted: true} // valid without erasure info + } + + disks := make([]StorageAPI, 16) + for i := range disks { + disks[i] = &renameDataFailingDisk{err: errCommitMetaMissing} + } + _, _, _, err := renameData(t.Context(), disks, minioMetaTmpBucket, "tmp", metadata, "bucket", "object", 9) + if !errors.Is(err, errErasureWriteQuorum) { + t.Fatalf("expected %s for in-quorum commit failure, got %v", errErasureWriteQuorum, err) + } + + for i := range disks { + disks[i] = &renameDataFailingDisk{err: errFileNotFound} + } + _, _, _, err = renameData(t.Context(), disks, minioMetaTmpBucket, "tmp", metadata, "bucket", "object", 9) + if !errors.Is(err, errFileNotFound) { + t.Fatalf("expected %s for in-quorum staging failure, got %v", errFileNotFound, err) + } +} + // TestHashOrder - test order of ints in array func TestHashOrder(t *testing.T) { testCases := []struct { From 3196480da93ebe7583677e64848cc5afa4dada18 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Wed, 14 Oct 2026 15:24:37 +0800 Subject: [PATCH 06/14] fix: enforce retention authorization and immutable version writes Close SN-2026-015 by requiring PutObjectRetention with request-derived condition keys before reading bucket or object state. Weakening unexpired GOVERNANCE retention also requires the bypass header and permission. Treat retention removal as zero remaining days and avoid duration saturation for distant dates. Close SN-2026-016 by requiring replication trust for caller-selected target version IDs and checking persisted multipart targets and locked null versions under the destination write lock, including across storage pools. Preserve versioning transitions, trusted replication and batch Snowball version IDs, and fail closed when lock state cannot be read. Add authorization, concurrent lock-update, multipart transition, cross-pool, configuration-failure and retention-boundary regression coverage. Validate with make test, focused race tests and the original independent probes. Developed with Codex (Astra). Co-authored-by: Codex Signed-off-by: Feng Ruohang (cherry picked from commit fe3935476b929e03193d7496a7448d760e635096) --- cmd/auth-handler.go | 45 -- cmd/batch-handlers.go | 6 +- cmd/bucket-handlers.go | 6 + cmd/bucket-object-lock.go | 176 +++-- cmd/erasure-multipart.go | 26 + cmd/erasure-object.go | 22 + ...erasure-server-pool-multipart-http_test.go | 1 + ...rasure-server-pool-put-conditional_test.go | 17 +- cmd/erasure-server-pool.go | 13 +- cmd/erasure-sets.go | 6 + cmd/object-api-options.go | 5 + cmd/object-handlers.go | 29 +- cmd/object-multipart-handlers.go | 2 +- cmd/object-retention-authz_test.go | 679 ++++++++++++++++++ cmd/object-retention-overwrite_test.go | 302 ++++++++ cmd/replication-tagging-order_test.go | 2 +- 16 files changed, 1205 insertions(+), 132 deletions(-) create mode 100644 cmd/object-retention-authz_test.go create mode 100644 cmd/object-retention-overwrite_test.go diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go index fa35d9058..a20a74505 100644 --- a/cmd/auth-handler.go +++ b/cmd/auth-handler.go @@ -28,13 +28,10 @@ import ( "mime" "net/http" "net/url" - "strconv" "strings" "sync/atomic" - "time" "github.com/minio/minio/internal/auth" - objectlock "github.com/minio/minio/internal/bucket/object/lock" "github.com/minio/minio/internal/etag" "github.com/minio/minio/internal/hash" xhttp "github.com/minio/minio/internal/http" @@ -733,48 +730,6 @@ func setAuthMiddleware(h http.Handler) http.Handler { }) } -func isPutRetentionAllowed(bucketName, objectName string, retDays int, retDate time.Time, retMode objectlock.RetMode, byPassSet bool, r *http.Request, cred auth.Credentials, owner bool) (s3Err APIErrorCode) { - var retSet bool - if cred.AccessKey == "" { - return ErrAccessDenied - } - - conditions := getConditionValues(r, "", cred) - conditions["object-lock-mode"] = []string{string(retMode)} - conditions["object-lock-retain-until-date"] = []string{retDate.UTC().Format(time.RFC3339)} - if retDays > 0 { - conditions["object-lock-remaining-retention-days"] = []string{strconv.Itoa(retDays)} - } - if retMode == objectlock.RetGovernance && byPassSet { - byPassSet = globalIAMSys.IsAllowed(policy.Args{ - AccountName: cred.AccessKey, - Groups: cred.Groups, - Action: policy.BypassGovernanceRetentionAction, - BucketName: bucketName, - ObjectName: objectName, - ConditionValues: conditions, - IsOwner: owner, - Claims: cred.Claims, - }) - } - if globalIAMSys.IsAllowed(policy.Args{ - AccountName: cred.AccessKey, - Groups: cred.Groups, - Action: policy.PutObjectRetentionAction, - BucketName: bucketName, - ConditionValues: conditions, - ObjectName: objectName, - IsOwner: owner, - Claims: cred.Claims, - }) { - retSet = true - } - if byPassSet || retSet { - return ErrNone - } - return ErrAccessDenied -} - // isPutActionAllowed - check if PUT operation is allowed on the resource, this // call verifies bucket policies and IAM policies, supports multi user // checks etc. diff --git a/cmd/batch-handlers.go b/cmd/batch-handlers.go index 69d2394f1..660e6adfa 100644 --- a/cmd/batch-handlers.go +++ b/cmd/batch-handlers.go @@ -583,7 +583,11 @@ func toObjectInfo(bucket, object string, objInfo minio.ObjectInfo) ObjectInfo { func (r BatchJobReplicateV1) writeAsArchive(ctx context.Context, objAPI ObjectLayer, remoteClnt *minio.Client, entries []ObjectInfo, prefix string) error { input := make(chan minio.SnowballObject, 1) opts := minio.SnowballOptions{ - Opts: minio.PutObjectOptions{}, + Opts: minio.PutObjectOptions{ + Internal: minio.AdvancedPutOptions{ + ReplicationRequest: true, + }, + }, InMemory: *r.Source.Snowball.InMemory, Compress: *r.Source.Snowball.Compress, SkipErrs: *r.Source.Snowball.SkipErrs, diff --git a/cmd/bucket-handlers.go b/cmd/bucket-handlers.go index e79a7841f..0df95f057 100644 --- a/cmd/bucket-handlers.go +++ b/cmd/bucket-handlers.go @@ -1273,6 +1273,12 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h writeErrorResponseHeadersOnly(w, toAPIError(ctx, err)) return } + if isDirObject(object) { + if _, _, _, s3Err := checkPutObjectLockAllowed(ctx, r, bucket, object, objectAPI.GetObjectInfo, ErrNone, ErrNone, false); s3Err != ErrNone { + writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Err), r.URL) + return + } + } opts.WantChecksum = checksum fanOutOpts := fanOutOptions{Checksum: checksum} diff --git a/cmd/bucket-object-lock.go b/cmd/bucket-object-lock.go index 36211c45d..80f65f615 100644 --- a/cmd/bucket-object-lock.go +++ b/cmd/bucket-object-lock.go @@ -20,8 +20,8 @@ package cmd import ( "context" "errors" - "math" "net/http" + "strconv" "strings" "time" @@ -164,76 +164,129 @@ func enforceRetentionBypassForDelete(ctx context.Context, r *http.Request, bucke return nil } -// enforceRetentionBypassForPut enforces whether an existing object under governance can be overwritten -// with governance bypass headers set in the request. -// Objects under site wide WORM cannot be overwritten. -// For objects in "Governance" mode, overwrite is allowed if a) object retention date is past OR -// governance bypass headers are set and user has governance bypass permissions. -// Objects in compliance mode can be overwritten only if retention date is being extended. No mode change is permitted. +// isRetentionActionAllowed evaluates action for a PutObjectRetention request +// with the object-lock condition keys derived from the requested retention. +// The keys come from the request, not from the stored version, so the +// handler can decide s3:PutObjectRetention before reading any object state. +func isRetentionActionAllowed(action policy.Action, bucket, object string, objRetention *objectlock.ObjectRetention, r *http.Request, cred auth.Credentials, owner bool) bool { + if cred.AccessKey == "" { + return false + } + t, err := objectlock.UTCNowNTP() + if err != nil { + return false + } + conditions := getConditionValues(r, "", cred) + conditions["object-lock-mode"] = []string{string(objRetention.Mode)} + conditions["object-lock-retain-until-date"] = []string{objRetention.RetainUntilDate.UTC().Format(time.RFC3339)} + // Relative days from now, for "object-lock-remaining-retention-days". + // Clearing retention () has a zero RetainUntilDate, which must + // evaluate as 0 remaining days rather than a large positive math.Abs delta + // from year 0001. + days := 0 + if !objRetention.RetainUntilDate.IsZero() && objRetention.RetainUntilDate.After(t) { + // Time.Sub saturates after about 292 years. Count seconds instead so + // maximum-retention policies also apply to distant RFC3339 dates. + seconds := objRetention.RetainUntilDate.Unix() - t.Unix() + days = int(seconds / 86400) + if seconds%86400 != 0 || objRetention.RetainUntilDate.Nanosecond() > t.Nanosecond() { + days++ + } + } + conditions["object-lock-remaining-retention-days"] = []string{strconv.Itoa(days)} + return globalIAMSys.IsAllowed(policy.Args{ + AccountName: cred.AccessKey, + Groups: cred.Groups, + Action: action, + BucketName: bucket, + ConditionValues: conditions, + ObjectName: object, + IsOwner: owner, + Claims: cred.Claims, + }) +} + +// enforceRetentionBypassForPut enforces the stored retention of oi against a +// PutObjectRetention request. The caller must already hold +// s3:PutObjectRetention; this only decides changes that depend on the stored +// state. Expired or absent retention may be changed freely. Unexpired +// GOVERNANCE retention may be shortened, removed or have its mode changed only +// with the bypass header and s3:BypassGovernanceRetention. Unexpired +// COMPLIANCE retention may only be extended. func enforceRetentionBypassForPut(ctx context.Context, r *http.Request, oi ObjectInfo, objRetention *objectlock.ObjectRetention, cred auth.Credentials, owner bool) error { - byPassSet := objectlock.IsObjectLockGovernanceBypassSet(r.Header) + ret := objectlock.GetObjectRetentionMeta(oi.UserDefined) + if !ret.Mode.Valid() { + return nil + } t, err := objectlock.UTCNowNTP() if err != nil { internalLogIf(ctx, err, logger.WarningKind) return ObjectLocked{Bucket: oi.Bucket, Object: oi.Name, VersionID: oi.VersionID} } + if ret.RetainUntilDate.Before(t) { + return nil + } - // Pass in relative days from current time, to additionally - // to verify "object-lock-remaining-retention-days" policy if any. - days := int(math.Ceil(math.Abs(objRetention.RetainUntilDate.Sub(t).Hours()) / 24)) - - ret := objectlock.GetObjectRetentionMeta(oi.UserDefined) - if ret.Mode.Valid() { - // Retention has expired you may change whatever you like. - if ret.RetainUntilDate.Before(t) { - apiErr := isPutRetentionAllowed(oi.Bucket, oi.Name, - days, objRetention.RetainUntilDate.Time, - objRetention.Mode, byPassSet, r, cred, - owner) - if apiErr == ErrAccessDenied { - return errAuthentication - } - return nil + weakens := objRetention.Mode != ret.Mode || objRetention.RetainUntilDate.Before(ret.RetainUntilDate.Time) + if !weakens { + return nil + } + switch ret.Mode { + case objectlock.RetGovernance: + // https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-lock.html#object-lock-bypass-governance + if !objectlock.IsObjectLockGovernanceBypassSet(r.Header) { + return ObjectLocked{Bucket: oi.Bucket, Object: oi.Name, VersionID: oi.VersionID} } - - switch ret.Mode { - case objectlock.RetGovernance: - govPerm := isPutRetentionAllowed(oi.Bucket, oi.Name, days, - objRetention.RetainUntilDate.Time, objRetention.Mode, - byPassSet, r, cred, owner) - // Governance mode retention period cannot be shortened, if x-amz-bypass-governance is not set. - if !byPassSet { - if objRetention.Mode != objectlock.RetGovernance || objRetention.RetainUntilDate.Before(ret.RetainUntilDate.Time) { - return ObjectLocked{Bucket: oi.Bucket, Object: oi.Name, VersionID: oi.VersionID} - } - } - if govPerm == ErrAccessDenied { - return errAuthentication - } - return nil - case objectlock.RetCompliance: - // Compliance retention mode cannot be changed or shortened. - // https://docs.aws.amazon.com/AmazonS3/latest/dev/object-lock-overview.html#object-lock-retention-modes - if objRetention.Mode != objectlock.RetCompliance || objRetention.RetainUntilDate.Before(ret.RetainUntilDate.Time) { - return ObjectLocked{Bucket: oi.Bucket, Object: oi.Name, VersionID: oi.VersionID} - } - apiErr := isPutRetentionAllowed(oi.Bucket, oi.Name, - days, objRetention.RetainUntilDate.Time, objRetention.Mode, - false, r, cred, owner) - if apiErr == ErrAccessDenied { - return errAuthentication - } - return nil + if !isRetentionActionAllowed(policy.BypassGovernanceRetentionAction, oi.Bucket, oi.Name, objRetention, r, cred, owner) { + return errAuthentication } return nil - } // No pre-existing retention metadata present. + default: + // Compliance retention mode cannot be changed or shortened. + // https://docs.aws.amazon.com/AmazonS3/latest/dev/object-lock-overview.html#object-lock-retention-modes + return ObjectLocked{Bucket: oi.Bucket, Object: oi.Name, VersionID: oi.VersionID} + } +} - apiErr := isPutRetentionAllowed(oi.Bucket, oi.Name, - days, objRetention.RetainUntilDate.Time, - objRetention.Mode, byPassSet, r, cred, owner) - if apiErr == ErrAccessDenied { - return errAuthentication +// checkObjectLockedForOverwrite re-checks under the object write lock whether an +// in-place write to the null version (such as a directory object) would +// overwrite a locked version in an Object Lock bucket. +func checkObjectLockedForOverwrite(ctx context.Context, bucket, object string, getObjectInfoFn GetObjectInfoFn) error { + if isMinioMetaBucketName(bucket) { + return nil + } + retentionCfg, err := globalBucketObjectLockSys.Get(bucket) + if err != nil { + return err + } + if !retentionCfg.LockEnabled { + return nil + } + objInfo, err := getObjectInfoFn(ctx, bucket, object, ObjectOptions{ + VersionID: nullVersionID, + NoLock: true, + NoAuditLog: true, + }) + if err != nil { + if isErrObjectNotFound(err) || isErrVersionNotFound(err) || errors.As(err, &MethodNotAllowed{}) { + return nil + } + return err + } + if objectlock.GetObjectLegalHoldMeta(objInfo.UserDefined).Status == objectlock.LegalHoldOn { + return ObjectLocked{Bucket: bucket, Object: decodeDirObject(object), VersionID: nullVersionID} + } + r := objectlock.GetObjectRetentionMeta(objInfo.UserDefined) + if r.Mode == objectlock.RetCompliance || r.Mode == objectlock.RetGovernance { + t, err := objectlock.UTCNowNTP() + if err != nil { + internalLogIf(ctx, err, logger.WarningKind) + return ObjectLocked{Bucket: bucket, Object: decodeDirObject(object), VersionID: nullVersionID} + } + if r.RetainUntilDate.After(t) { + return ObjectLocked{Bucket: bucket, Object: decodeDirObject(object), VersionID: nullVersionID} + } } return nil } @@ -274,6 +327,9 @@ func checkPutObjectLockAllowed(ctx context.Context, rq *http.Request, bucket, ob if err != nil { return mode, retainDate, legalHold, toAPIErrorCode(ctx, err) } + if isDirObject(object) && opts.VersionID == "" { + opts.VersionID = nullVersionID + } if opts.VersionID != "" && !replicaTrusted { if objInfo, err := getObjectInfoFn(ctx, bucket, object, opts); err == nil { @@ -283,7 +339,7 @@ func checkPutObjectLockAllowed(ctx context.Context, rq *http.Request, bucket, ob internalLogIf(ctx, err, logger.WarningKind) return mode, retainDate, legalHold, ErrObjectLocked } - if r.Mode == objectlock.RetCompliance && r.RetainUntilDate.After(t) { + if (r.Mode == objectlock.RetCompliance || r.Mode == objectlock.RetGovernance) && r.RetainUntilDate.After(t) { return mode, retainDate, legalHold, ErrObjectLocked } mode = r.Mode diff --git a/cmd/erasure-multipart.go b/cmd/erasure-multipart.go index 89355c762..7da35c014 100644 --- a/cmd/erasure-multipart.go +++ b/cmd/erasure-multipart.go @@ -1696,6 +1696,32 @@ func (er erasureObjects) CompleteMultipartUpload(ctx context.Context, bucket str defer lk.Unlock(lkctx) } + if !opts.ReplicationRequest && !opts.ReplicaLockReconcile && !opts.DataMovement { + getObjectInfo := er.getObjectInfo + if opts.replicaObjectInfo != nil { + getObjectInfo = opts.replicaObjectInfo + } + if fi.VersionID == "" || fi.VersionID == nullVersionID { + if err := checkObjectLockedForOverwrite(ctx, bucket, object, getObjectInfo); err != nil { + return oi, toObjectErr(err, bucket, object) + } + } else { + _, gerr := getObjectInfo(ctx, bucket, object, ObjectOptions{ + VersionID: fi.VersionID, + Versioned: opts.Versioned, + VersionSuspended: opts.VersionSuspended, + NoLock: true, + }) + switch { + case gerr == nil, errors.As(gerr, &MethodNotAllowed{}): + return oi, toObjectErr(errAuthentication, bucket, object) + case isErrVersionNotFound(gerr) || isErrObjectNotFound(gerr): + default: + return oi, toObjectErr(gerr, bucket, object) + } + } + } + er.cleanupMultipartPath(ctx, paths...) // cleanup all part.N.meta, and skipped part.N's before final rename(). defer func() { diff --git a/cmd/erasure-object.go b/cmd/erasure-object.go index 8ab3d17a1..a373ea543 100644 --- a/cmd/erasure-object.go +++ b/cmd/erasure-object.go @@ -150,6 +150,17 @@ func (er erasureObjects) CopyObject(ctx context.Context, srcBucket, srcObject, d } } + if !dstOpts.ReplicationRequest && !dstOpts.ReplicaLockReconcile && !dstOpts.DataMovement && + (versionID == "" || versionID == nullVersionID) { + getObjectInfo := er.getObjectInfo + if dstOpts.replicaObjectInfo != nil { + getObjectInfo = dstOpts.replicaObjectInfo + } + if err := checkObjectLockedForOverwrite(ctx, dstBucket, dstObject, getObjectInfo); err != nil { + return oi, toObjectErr(err, dstBucket, dstObject) + } + } + modTime = UTCNow() // We only preserve modTime if dstOpts.MTime is true. // in all other cases mtime is latest. @@ -1594,6 +1605,17 @@ func (er erasureObjects) putObject(ctx context.Context, bucket string, object st defer lk.Unlock(lkctx) } + if !opts.ReplicationRequest && !opts.ReplicaLockReconcile && !opts.DataMovement && + (fi.VersionID == "" || fi.VersionID == nullVersionID) { + getObjectInfo := er.getObjectInfo + if opts.replicaObjectInfo != nil { + getObjectInfo = opts.replicaObjectInfo + } + if err := checkObjectLockedForOverwrite(ctx, bucket, object, getObjectInfo); err != nil { + return ObjectInfo{}, toObjectErr(err, bucket, object) + } + } + // Rename the successfully written temporary object to final location. onlineDisks, versions, oldDataDir, err := renameData(ctx, onlineDisks, minioMetaTmpBucket, tempObj, partsMetadata, bucket, object, writeQuorum) if err != nil { diff --git a/cmd/erasure-server-pool-multipart-http_test.go b/cmd/erasure-server-pool-multipart-http_test.go index ed2e771e1..f1a7c84c3 100644 --- a/cmd/erasure-server-pool-multipart-http_test.go +++ b/cmd/erasure-server-pool-multipart-http_test.go @@ -265,6 +265,7 @@ func TestPoolsMultipartConditionalLatestVersionAndCallbackOnce(t *testing.T) { if explicit { current = putConsistencyObject(t, z, bucket, object, 1, "latest-other-version", ObjectOptions{Versioned: true, MTime: UTCNow().Add(-time.Minute)}) } + opts.VersionID = "" id, parts := multipartConditionUpload(t, z, bucket, object, 1, opts) called := 0 opts.CheckPrecondFn = func(oi ObjectInfo) bool { called++; return oi.ETag != current.ETag } diff --git a/cmd/erasure-server-pool-put-conditional_test.go b/cmd/erasure-server-pool-put-conditional_test.go index 4bd576037..9f7317c4d 100644 --- a/cmd/erasure-server-pool-put-conditional_test.go +++ b/cmd/erasure-server-pool-put-conditional_test.go @@ -633,6 +633,8 @@ func TestPoolsConditionalPutReplicaAvailability(t *testing.T) { } } +// A public client cannot address a destination version, with or without a +// precondition; only trusted replication may (SN-2026-016). func TestPoolsConditionalPutDestinationVersionHTTP(t *testing.T) { z, _ := consistencyPools(t) bucket, router := conditionalPutBucket(t, z, "versioned") @@ -641,21 +643,14 @@ func TestPoolsConditionalPutDestinationVersionHTTP(t *testing.T) { current := putConsistencyObject(t, z, bucket, object, 1, "current", ObjectOptions{Versioned: true, MTime: UTCNow().Add(-time.Minute)}) defer conditionalPutPool(t, z, object, 0)() url := getPutObjectURL("", bucket, object) + "?versionId=" + old.VersionID - for _, stale := range []bool{true, false} { - tag, status := current.ETag, http.StatusOK - if stale { - tag, status = old.ETag, http.StatusPreconditionFailed - } + for _, tag := range []string{old.ETag, current.ETag} { put := multipartConditionRequest(t, router, http.MethodPut, url, "replacement", map[string]string{xhttp.IfMatch: fmt.Sprintf("%q", tag)}) - if put.Code != status { - t.Fatalf("version-addressed public PUT: %d want %d: %s", put.Code, status, put.Body.String()) - } - if got := put.Header()[xhttp.AmzVersionID]; !stale && (len(got) != 1 || got[0] != old.VersionID) { - t.Fatalf("write version changed: %v", put.Header()) + if put.Code != http.StatusForbidden { + t.Fatalf("version-addressed public PUT: %d want 403: %s", put.Code, put.Body.String()) } } get := multipartConditionRequest(t, router, http.MethodGet, url, "", nil) - if get.Code != http.StatusOK || get.Body.String() != "replacement" { + if get.Code != http.StatusOK || get.Body.String() != "old" { t.Fatalf("addressed GET: %d %q", get.Code, get.Body.String()) } } diff --git a/cmd/erasure-server-pool.go b/cmd/erasure-server-pool.go index df1d2f22d..137204f97 100644 --- a/cmd/erasure-server-pool.go +++ b/cmd/erasure-server-pool.go @@ -1199,6 +1199,10 @@ func (z *erasureServerPools) PutObject(ctx context.Context, bucket string, objec if opts.ReplicaLockReconcile || opts.DataMovement { opts.ReplicaLockReconcile = true + } + // Public overwrites need all pools for Object Lock. Unaddressed replica + // callbacks retain their set-local reads unless reconciliation is needed. + if opts.ReplicaLockReconcile || !opts.ReplicationRequest { opts.replicaObjectInfo = z.replicaObjectInfo } oi, err := z.serverPools[idx].PutObject(ctx, bucket, object, data, opts) @@ -1510,9 +1514,9 @@ func (z *erasureServerPools) CopyObject(ctx context.Context, srcBucket, srcObjec return objInfo, err } - if !z.SinglePool() && dstOpts.ReplicaLockReconcile { + if !z.SinglePool() && (dstOpts.ReplicaLockReconcile || !dstOpts.ReplicationRequest) { dstOpts.replicaObjectInfo = z.replicaObjectInfo - if !dstOpts.DataMovement { + if dstOpts.ReplicaLockReconcile && !dstOpts.DataMovement { defer func() { if err == nil { err = z.retireReplicaCopies(ctx, dstBucket, dstObject, poolIdx, objInfo) @@ -1549,13 +1553,16 @@ func (z *erasureServerPools) CopyObject(ctx context.Context, srcBucket, srcObjec ServerSideEncryption: dstOpts.ServerSideEncryption, UserDefined: srcInfo.UserDefined, Versioned: dstOpts.Versioned, + VersionSuspended: dstOpts.VersionSuspended, VersionID: dstOpts.VersionID, MTime: dstOpts.MTime, NoLock: true, EncryptFn: dstOpts.EncryptFn, WantChecksum: dstOpts.WantChecksum, WantServerSideChecksumType: dstOpts.WantServerSideChecksumType, + ReplicationRequest: dstOpts.ReplicationRequest, ReplicaLockReconcile: dstOpts.ReplicaLockReconcile, + DataMovement: dstOpts.DataMovement, replicaObjectInfo: dstOpts.replicaObjectInfo, } @@ -2305,6 +2312,8 @@ func (z *erasureServerPools) CompleteMultipartUpload(ctx context.Context, bucket if opts.ReplicaLockReconcile || opts.DataMovement { opts.ReplicaLockReconcile = true + } + if opts.ReplicaLockReconcile || !opts.ReplicationRequest { opts.replicaObjectInfo = z.replicaObjectInfo } diff --git a/cmd/erasure-sets.go b/cmd/erasure-sets.go index f655804cd..d8b4dcbf6 100644 --- a/cmd/erasure-sets.go +++ b/cmd/erasure-sets.go @@ -869,11 +869,17 @@ func (s *erasureSets) CopyObject(ctx context.Context, srcBucket, srcObject, dstB ServerSideEncryption: dstOpts.ServerSideEncryption, UserDefined: srcInfo.UserDefined, Versioned: dstOpts.Versioned, + VersionSuspended: dstOpts.VersionSuspended, VersionID: dstOpts.VersionID, MTime: dstOpts.MTime, + NoLock: dstOpts.NoLock, EncryptFn: dstOpts.EncryptFn, WantChecksum: dstOpts.WantChecksum, WantServerSideChecksumType: dstOpts.WantServerSideChecksumType, + ReplicationRequest: dstOpts.ReplicationRequest, + ReplicaLockReconcile: dstOpts.ReplicaLockReconcile, + DataMovement: dstOpts.DataMovement, + replicaObjectInfo: dstOpts.replicaObjectInfo, } return dstSet.putObject(ctx, dstBucket, dstObject, srcInfo.PutObjReader, putOpts) diff --git a/cmd/object-api-options.go b/cmd/object-api-options.go index 930808eae..9aeee35fd 100644 --- a/cmd/object-api-options.go +++ b/cmd/object-api-options.go @@ -347,6 +347,11 @@ func putOpts(ctx context.Context, bucket, object, vid string, hdrs http.Header, versionSuspended := globalBucketVersioningSys.PrefixSuspended(bucket, object) vid = strings.TrimSpace(vid) + // A caller-chosen version ID writes that version in place. Only trusted + // replication may recreate a source version under its original ID. + if vid != "" && !trustedReplication { + return opts, errAuthentication + } if vid != "" && vid != nullVersionID { _, err := uuid.Parse(vid) if err != nil { diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index fa44129af..26784ac16 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -3435,27 +3435,16 @@ func (api objectAPIHandlers) PutObjectRetentionHandler(w http.ResponseWriter, r return } - // Check permissions to perform this object retention operation if s3Error := authenticateRequest(ctx, r, policy.PutObjectRetentionAction); s3Error != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL) return } - if _, err := objectAPI.GetBucketInfo(ctx, bucket, BucketOptions{}); err != nil { - writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) - return - } - if !validateLengthAndChecksum(r) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrMissingContentMD5), r.URL) return } - if rcfg, _ := globalBucketObjectLockSys.Get(bucket); !rcfg.LockEnabled { - writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrInvalidBucketObjectLockConfiguration), r.URL) - return - } - objRetention, err := objectlock.ParseObjectRetention(r.Body) if err != nil { apiErr := errorCodes.ToAPIErr(ErrMalformedXML) @@ -3467,6 +3456,24 @@ func (api objectAPIHandlers) PutObjectRetentionHandler(w http.ResponseWriter, r reqInfo := logger.GetReqInfo(ctx) reqInfo.SetTags("retention", objRetention.String()) + // Check permissions to perform this object retention operation. The + // object-lock condition keys come from the request body, so this is + // decided before any bucket or object state is read. + if !isRetentionActionAllowed(policy.PutObjectRetentionAction, bucket, object, objRetention, r, reqInfo.Cred, reqInfo.Owner) { + writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrAccessDenied), r.URL) + return + } + + if _, err := objectAPI.GetBucketInfo(ctx, bucket, BucketOptions{}); err != nil { + writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) + return + } + + if rcfg, _ := globalBucketObjectLockSys.Get(bucket); !rcfg.LockEnabled { + writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrInvalidBucketObjectLockConfiguration), r.URL) + return + } + opts, err := getOpts(ctx, r, bucket, object) if err != nil { writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) diff --git a/cmd/object-multipart-handlers.go b/cmd/object-multipart-handlers.go index e56630005..6ffccf759 100644 --- a/cmd/object-multipart-handlers.go +++ b/cmd/object-multipart-handlers.go @@ -1166,7 +1166,7 @@ func (api objectAPIHandlers) CompleteMultipartUploadHandler(w http.ResponseWrite return } - if _, _, _, s3Err := checkPutObjectLockAllowed(ctx, r, bucket, object, objectAPI.GetObjectInfo, ErrNone, ErrNone, false); s3Err != ErrNone { + if _, _, _, s3Err := checkPutObjectLockAllowed(ctx, r, bucket, object, objectAPI.GetObjectInfo, ErrNone, ErrNone, trustedReplication); s3Err != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Err), r.URL) return } diff --git a/cmd/object-retention-authz_test.go b/cmd/object-retention-authz_test.go new file mode 100644 index 000000000..af44aac37 --- /dev/null +++ b/cmd/object-retention-authz_test.go @@ -0,0 +1,679 @@ +// Copyright (c) 2015-2026 MinIO, Inc. +// Copyright (c) 2026 PGSTY +// +// This file is part of MinIO Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +package cmd + +import ( + "archive/tar" + "bytes" + "context" + "crypto/md5" + "encoding/base64" + "encoding/xml" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + "github.com/minio/madmin-go/v3" + "github.com/minio/minio/internal/amztime" + "github.com/minio/minio/internal/auth" + objectlock "github.com/minio/minio/internal/bucket/object/lock" + xhttp "github.com/minio/minio/internal/http" + "github.com/pgsty/silo-pkg/v3/policy" +) + +func retentionAuthzRequest(t *testing.T, router http.Handler, creds auth.Credentials, method, target string, body []byte, headers map[string]string) *httptest.ResponseRecorder { + t.Helper() + var rd io.ReadSeeker + if body != nil { + rd = bytes.NewReader(body) + } + req, err := newTestSignedRequestV4(method, target, int64(len(body)), rd, creds.AccessKey, creds.SecretKey, headers) + if err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + return rec +} + +func retentionBody(mode string, until time.Time) []byte { + if mode == "" { + return []byte(``) + } + return fmt.Appendf(nil, `%s%s`, + mode, until.UTC().Format(time.RFC3339)) +} + +func putRetention(t *testing.T, router http.Handler, cred auth.Credentials, bucket, key, vid, mode string, until time.Time, bypass bool) int { + t.Helper() + body := retentionBody(mode, until) + sum := md5.Sum(body) + headers := map[string]string{xhttp.ContentMD5: base64.StdEncoding.EncodeToString(sum[:])} + if bypass { + headers[objectlock.AmzObjectLockBypassRetGovernance] = "true" + } + rec := retentionAuthzRequest(t, router, cred, http.MethodPut, + makeTestTargetURL("", bucket, key, url.Values{"retention": {""}, "versionId": {vid}}), body, headers) + return rec.Code +} + +func newRetentionPolicyUser(t *testing.T, instanceType, policyJSON string) auth.Credentials { + t.Helper() + ctx := t.Context() + accessKey, secretKey, err := auth.GenerateCredentials() + if err != nil { + t.Fatalf("%s: generate credentials: %v", instanceType, err) + } + creds := auth.Credentials{AccessKey: accessKey, SecretKey: secretKey} + if _, err = globalIAMSys.CreateUser(ctx, creds.AccessKey, madmin.AddOrUpdateUserReq{ + SecretKey: creds.SecretKey, + Status: madmin.AccountEnabled, + }); err != nil { + t.Fatalf("%s: create user: %v", instanceType, err) + } + parsed, err := policy.ParseConfig(strings.NewReader(policyJSON)) + if err != nil { + t.Fatalf("%s: parse policy: %v", instanceType, err) + } + policyName := "retention-authz-" + mustGetUUID() + if _, err = globalIAMSys.SetPolicy(ctx, policyName, *parsed); err != nil { + t.Fatalf("%s: install policy: %v", instanceType, err) + } + if _, err = globalIAMSys.PolicyDBSet(ctx, creds.AccessKey, policyName, regUser, false); err != nil { + t.Fatalf("%s: attach policy: %v", instanceType, err) + } + return creds +} + +// TestAPIPutObjectRetentionAuthorization covers SN-2026-015: the governance +// bypass header is intent only; s3:PutObjectRetention is always required and +// weakening unexpired GOVERNANCE also requires s3:BypassGovernanceRetention. +func TestAPIPutObjectRetentionAuthorization(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPIPutObjectRetentionAuthorization, + }) +} + +func testAPIPutObjectRetentionAuthorization(obj ObjectLayer, instanceType, bucket string, router http.Handler, root auth.Credentials, t *testing.T) { + enableBucketObjectLock(t, bucket) + + noRet := newObjectAttributesAuthzUser(t, instanceType, bucket, `"s3:GetObject"`) + retOnly := newObjectAttributesAuthzUser(t, instanceType, bucket, `"s3:PutObjectRetention"`) + retBypass := newObjectAttributesAuthzUser(t, instanceType, bucket, `"s3:PutObjectRetention","s3:BypassGovernanceRetention"`) + bypassOnly := newObjectAttributesAuthzUser(t, instanceType, bucket, `"s3:BypassGovernanceRetention"`) + condDenyShort := newRetentionPolicyUser(t, instanceType, `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["s3:PutObjectRetention", "s3:BypassGovernanceRetention"], + "Resource": ["arn:aws:s3:::`+bucket+`/*"] + }, + { + "Effect": "Deny", + "Action": ["s3:PutObjectRetention"], + "Resource": ["arn:aws:s3:::`+bucket+`/*"], + "Condition": {"NumericLessThan": {"s3:object-lock-remaining-retention-days": "60"}} + } + ] + }`) + explicitDenyBypass := newRetentionPolicyUser(t, instanceType, `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["s3:*"], + "Resource": ["arn:aws:s3:::`+bucket+`/*"] + }, + { + "Effect": "Deny", + "Action": ["s3:BypassGovernanceRetention"], + "Resource": ["arn:aws:s3:::`+bucket+`/*"] + } + ] + }`) + + far := UTCNow().Add(365 * 24 * time.Hour).Truncate(time.Second) + nearer := UTCNow().Add(30 * 24 * time.Hour).Truncate(time.Second) + farther := far.Add(24 * time.Hour) + + put := func(t *testing.T, object, mode string) string { + t.Helper() + payload := []byte("retention authorization") + opts := ObjectOptions{Versioned: true, UserDefined: map[string]string{}} + if mode != "" { + opts.UserDefined[strings.ToLower(xhttp.AmzObjectLockMode)] = mode + opts.UserDefined[strings.ToLower(xhttp.AmzObjectLockRetainUntilDate)] = amztime.ISO8601Format(far) + } + info, err := obj.PutObject(t.Context(), bucket, object, + mustGetPutObjReader(t, bytes.NewReader(payload), int64(len(payload)), "", ""), opts) + if err != nil { + t.Fatal(err) + } + return info.VersionID + } + stored := func(t *testing.T, object, vid string) objectlock.ObjectRetention { + t.Helper() + info, err := obj.GetObjectInfo(t.Context(), bucket, object, ObjectOptions{VersionID: vid}) + if err != nil { + t.Fatal(err) + } + return objectlock.GetObjectRetentionMeta(info.UserDefined) + } + + for _, test := range []struct { + name string + creds auth.Credentials + existing string // stored retention mode on the version, until far + mode string // requested mode, "" removes retention + until time.Time + bypass bool + want int + changed bool + }{ + {name: "no-permission removal", creds: noRet, existing: "GOVERNANCE", bypass: true, want: http.StatusForbidden}, + {name: "no-permission mode change", creds: noRet, existing: "GOVERNANCE", mode: "COMPLIANCE", until: nearer, bypass: true, want: http.StatusForbidden}, + {name: "no-permission new compliance", creds: noRet, mode: "COMPLIANCE", until: nearer, bypass: true, want: http.StatusForbidden}, + {name: "retention-only shorten with header", creds: retOnly, existing: "GOVERNANCE", mode: "GOVERNANCE", until: nearer, bypass: true, want: http.StatusForbidden}, + {name: "retention-only removal with header", creds: retOnly, existing: "GOVERNANCE", bypass: true, want: http.StatusForbidden}, + {name: "retention-only mode change with header", creds: retOnly, existing: "GOVERNANCE", mode: "COMPLIANCE", until: nearer, bypass: true, want: http.StatusForbidden}, + {name: "retention-only shorten without header", creds: retOnly, existing: "GOVERNANCE", mode: "GOVERNANCE", until: nearer, want: http.StatusBadRequest}, + {name: "retention-only extend with header", creds: retOnly, existing: "GOVERNANCE", mode: "GOVERNANCE", until: farther, bypass: true, want: http.StatusOK, changed: true}, + {name: "retention-only new retention", creds: retOnly, mode: "GOVERNANCE", until: nearer, want: http.StatusOK, changed: true}, + {name: "bypass shorten with header", creds: retBypass, existing: "GOVERNANCE", mode: "GOVERNANCE", until: nearer, bypass: true, want: http.StatusOK, changed: true}, + {name: "bypass removal with header", creds: retBypass, existing: "GOVERNANCE", bypass: true, want: http.StatusOK, changed: true}, + {name: "bypass shorten without header", creds: retBypass, existing: "GOVERNANCE", mode: "GOVERNANCE", until: nearer, want: http.StatusBadRequest}, + {name: "bypass shorten compliance", creds: retBypass, existing: "COMPLIANCE", mode: "COMPLIANCE", until: nearer, bypass: true, want: http.StatusBadRequest}, + {name: "bypass-only extend", creds: bypassOnly, existing: "GOVERNANCE", mode: "GOVERNANCE", until: farther, bypass: true, want: http.StatusForbidden}, + {name: "conditional-deny-put-retention with bypass header", creds: condDenyShort, existing: "GOVERNANCE", mode: "GOVERNANCE", until: nearer, bypass: true, want: http.StatusForbidden}, + {name: "conditional-allow-put-retention satisfied", creds: condDenyShort, existing: "GOVERNANCE", mode: "GOVERNANCE", until: farther, bypass: true, want: http.StatusOK, changed: true}, + {name: "explicit-deny-bypass shorten with header", creds: explicitDenyBypass, existing: "GOVERNANCE", mode: "GOVERNANCE", until: nearer, bypass: true, want: http.StatusForbidden}, + {name: "owner shorten with header", creds: root, existing: "GOVERNANCE", mode: "GOVERNANCE", until: nearer, bypass: true, want: http.StatusOK, changed: true}, + } { + t.Run(instanceType+"/"+test.name, func(t *testing.T) { + object := "retention-authz/" + strings.ReplaceAll(test.name, " ", "-") + vid := put(t, object, test.existing) + before := stored(t, object, vid) + + body := retentionBody(test.mode, test.until) + sum := md5.Sum(body) + headers := map[string]string{xhttp.ContentMD5: base64.StdEncoding.EncodeToString(sum[:])} + if test.bypass { + headers[objectlock.AmzObjectLockBypassRetGovernance] = "true" + } + target := makeTestTargetURL("", bucket, object, url.Values{"retention": {""}, "versionId": {vid}}) + rec := retentionAuthzRequest(t, router, test.creds, http.MethodPut, target, body, headers) + if rec.Code != test.want { + t.Fatalf("status = %d, want %d: %s", rec.Code, test.want, rec.Body.String()) + } + after := stored(t, object, vid) + if changed := after.Mode != before.Mode || !after.RetainUntilDate.Equal(before.RetainUntilDate.Time); changed != test.changed { + t.Fatalf("retention changed = %v, want %v (before %s, after %s)", changed, test.changed, before.String(), after.String()) + } + }) + } + + t.Run(instanceType+"/no-permission missing bucket", func(t *testing.T) { + body := retentionBody("", time.Time{}) + sum := md5.Sum(body) + target := makeTestTargetURL("", "retention-authz-missing", "object", url.Values{"retention": {""}}) + rec := retentionAuthzRequest(t, router, noRet, http.MethodPut, target, body, + map[string]string{xhttp.ContentMD5: base64.StdEncoding.EncodeToString(sum[:])}) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403 before bucket lookup: %s", rec.Code, rec.Body.String()) + } + }) +} + +// TestAPIVersionAddressedWriteRequiresReplicationTrust covers SN-2026-016: a +// caller-chosen destination versionId is honored only for trusted replication, +// and write paths cannot overwrite locked versions via directory keys or +// version-addressed multipart completions. +func TestAPIVersionAddressedWriteRequiresReplicationTrust(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPIVersionAddressedWriteRequiresReplicationTrust, + makeBucketOptions: MakeBucketOptions{VersioningEnabled: true, LockEnabled: true}, + }) +} + +func testAPIVersionAddressedWriteRequiresReplicationTrust(obj ObjectLayer, instanceType, bucket string, router http.Handler, root auth.Credentials, t *testing.T) { + writer := newObjectAttributesAuthzUser(t, instanceType, bucket, `"s3:GetObject","s3:PutObject"`) + original := []byte("original version content") + info, err := obj.PutObject(t.Context(), bucket, "addressed", + mustGetPutObjReader(t, bytes.NewReader(original), int64(len(original)), "", ""), ObjectOptions{Versioned: true}) + if err != nil { + t.Fatal(err) + } + vid := info.VersionID + + unchanged := func(t *testing.T) { + t.Helper() + got, err := obj.GetObjectInfo(t.Context(), bucket, "addressed", ObjectOptions{VersionID: vid}) + if err != nil { + t.Fatal(err) + } + if got.Size != int64(len(original)) || got.ETag != info.ETag { + t.Fatalf("version %s was rewritten: size %d etag %s", vid, got.Size, got.ETag) + } + } + + for _, creds := range []struct { + name string + creds auth.Credentials + }{{"writer", writer}, {"owner", root}} { + t.Run(instanceType+"/"+creds.name+"/put", func(t *testing.T) { + body := []byte("replacement") + target := makeTestTargetURL("", bucket, "addressed", url.Values{"versionId": {vid}}) + rec := retentionAuthzRequest(t, router, creds.creds, http.MethodPut, target, body, nil) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403: %s", rec.Code, rec.Body.String()) + } + unchanged(t) + }) + t.Run(instanceType+"/"+creds.name+"/null-version", func(t *testing.T) { + body := []byte("replacement") + target := makeTestTargetURL("", bucket, "addressed", url.Values{"versionId": {nullVersionID}}) + rec := retentionAuthzRequest(t, router, creds.creds, http.MethodPut, target, body, nil) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403: %s", rec.Code, rec.Body.String()) + } + }) + t.Run(instanceType+"/"+creds.name+"/copy", func(t *testing.T) { + target := makeTestTargetURL("", bucket, "addressed", url.Values{"versionId": {vid}}) + req, err := newTestSignedRequestV4(http.MethodPut, target, 0, nil, creds.creds.AccessKey, creds.creds.SecretKey, + map[string]string{xhttp.AmzCopySource: "/" + bucket + "/addressed?versionId=" + vid, xhttp.AmzMetadataDirective: "REPLACE"}) + if err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403: %s", rec.Code, rec.Body.String()) + } + unchanged(t) + }) + t.Run(instanceType+"/"+creds.name+"/multipart", func(t *testing.T) { + target := makeTestTargetURL("", bucket, "addressed", url.Values{"uploads": {""}, "versionId": {vid}}) + rec := retentionAuthzRequest(t, router, creds.creds, http.MethodPost, target, nil, nil) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403: %s", rec.Code, rec.Body.String()) + } + }) + t.Run(instanceType+"/"+creds.name+"/complete-multipart-existing-version", func(t *testing.T) { + // Simulate a pending upload persisted with an existing version ID + // (e.g. initiated pre-upgrade or by a replication peer). + up, err := obj.NewMultipartUpload(t.Context(), bucket, "addressed", ObjectOptions{ + Versioned: true, + VersionID: vid, + }) + if err != nil { + t.Fatal(err) + } + defer obj.AbortMultipartUpload(t.Context(), bucket, "addressed", up.UploadID, ObjectOptions{}) + + partPayload := []byte("replacement-part") + pi, err := obj.PutObjectPart(t.Context(), bucket, "addressed", up.UploadID, 1, + mustGetPutObjReader(t, bytes.NewReader(partPayload), int64(len(partPayload)), "", ""), ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + completeBody := fmt.Appendf(nil, `1"%s"`, pi.ETag) + target := makeTestTargetURL("", bucket, "addressed", url.Values{"uploadId": {up.UploadID}}) + rec := retentionAuthzRequest(t, router, creds.creds, http.MethodPost, target, completeBody, nil) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403: %s", rec.Code, rec.Body.String()) + } + unchanged(t) + }) + } + + t.Run(instanceType+"/locked-directory-object", func(t *testing.T) { + far := UTCNow().Add(365 * 24 * time.Hour).Truncate(time.Second) + for _, lockCase := range []struct { + name string + meta map[string]string + }{ + { + name: "governance", + meta: map[string]string{ + strings.ToLower(xhttp.AmzObjectLockMode): "GOVERNANCE", + strings.ToLower(xhttp.AmzObjectLockRetainUntilDate): amztime.ISO8601Format(far), + }, + }, + { + name: "compliance", + meta: map[string]string{ + strings.ToLower(xhttp.AmzObjectLockMode): "COMPLIANCE", + strings.ToLower(xhttp.AmzObjectLockRetainUntilDate): amztime.ISO8601Format(far), + }, + }, + { + name: "legal-hold", + meta: map[string]string{ + strings.ToLower(xhttp.AmzObjectLockLegalHold): "ON", + }, + }, + } { + t.Run(lockCase.name, func(t *testing.T) { + dirKey := "locked-dir-" + lockCase.name + "/" + if _, err := obj.PutObject(t.Context(), bucket, dirKey, + mustGetPutObjReader(t, bytes.NewReader(nil), 0, "", ""), + ObjectOptions{Versioned: true, VersionID: nullVersionID, UserDefined: lockCase.meta}); err != nil { + t.Fatal(err) + } + target := makeTestTargetURL("", bucket, dirKey, nil) + rec := retentionAuthzRequest(t, router, writer, http.MethodPut, target, nil, nil) + if rec.Code != http.StatusBadRequest { + t.Fatalf("PUT locked dir status = %d, want 400: %s", rec.Code, rec.Body.String()) + } + got, err := obj.GetObjectInfo(t.Context(), bucket, dirKey, ObjectOptions{VersionID: nullVersionID}) + if err != nil { + t.Fatal(err) + } + for k, wantVal := range lockCase.meta { + if got.UserDefined[k] != wantVal { + t.Fatalf("metadata %s = %q, want %q", k, got.UserDefined[k], wantVal) + } + } + }) + } + }) + + t.Run(instanceType+"/snowball-archive-version-id", func(t *testing.T) { + var tarBuf bytes.Buffer + tw := tar.NewWriter(&tarBuf) + entryData := []byte("snowball-overwrite") + if err := tw.WriteHeader(&tar.Header{ + Typeflag: tar.TypeReg, + Name: "addressed", + Size: int64(len(entryData)), + Format: tar.FormatPAX, + PAXRecords: map[string]string{"minio.versionId": vid}, + }); err != nil { + t.Fatal(err) + } + if _, err := tw.Write(entryData); err != nil { + t.Fatal(err) + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + + // Untrusted writer cannot overwrite an existing version via PAX minio.versionId. + target := makeTestTargetURL("", bucket, "archive.tar", nil) + rec := retentionAuthzRequest(t, router, writer, http.MethodPut, target, tarBuf.Bytes(), + map[string]string{"x-amz-meta-snowball-auto-extract": "true"}) + if rec.Code != http.StatusForbidden { + t.Fatalf("untrusted snowball status = %d, want 403: %s", rec.Code, rec.Body.String()) + } + unchanged(t) + + // Trusted batch replication (with outer MinIOSourceReplicationRequest) preserves PAX minio.versionId. + replicaVID := mustGetUUID() + var repBuf bytes.Buffer + repTW := tar.NewWriter(&repBuf) + repData := []byte("replicated-via-snowball") + if err := repTW.WriteHeader(&tar.Header{ + Typeflag: tar.TypeReg, + Name: "snowball-replicated", + Size: int64(len(repData)), + Format: tar.FormatPAX, + PAXRecords: map[string]string{"minio.versionId": replicaVID}, + }); err != nil { + t.Fatal(err) + } + if _, err := repTW.Write(repData); err != nil { + t.Fatal(err) + } + if err := repTW.Close(); err != nil { + t.Fatal(err) + } + repRec := retentionAuthzRequest(t, router, root, http.MethodPut, target, repBuf.Bytes(), + map[string]string{ + "x-amz-meta-snowball-auto-extract": "true", + xhttp.MinIOSourceReplicationRequest: "true", + }) + if repRec.Code != http.StatusOK { + t.Fatalf("trusted snowball status = %d, want 200: %s", repRec.Code, repRec.Body.String()) + } + if _, err := obj.GetObjectInfo(t.Context(), bucket, "snowball-replicated", ObjectOptions{VersionID: replicaVID}); err != nil { + t.Fatalf("trusted snowball replication did not preserve version ID: %v", err) + } + }) + + t.Run(instanceType+"/trusted replication", func(t *testing.T) { + replicaVID := mustGetUUID() + body := []byte("replicated version") + target := makeTestTargetURL("", bucket, "replicated", url.Values{"versionId": {replicaVID}}) + rec := retentionAuthzRequest(t, router, root, http.MethodPut, target, body, + map[string]string{xhttp.MinIOSourceReplicationRequest: "true"}) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200: %s", rec.Code, rec.Body.String()) + } + if _, err := obj.GetObjectInfo(t.Context(), bucket, "replicated", ObjectOptions{VersionID: replicaVID}); err != nil { + t.Fatalf("trusted replication did not keep its version ID: %v", err) + } + }) +} + +func TestPutObjectRetentionClearMinimumDaysPolicy(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + makeBucketOptions: MakeBucketOptions{VersioningEnabled: true, LockEnabled: true}, + objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, root auth.Credentials, t *testing.T) { + for _, conditionalAllow := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/conditional-allow-%v", instance, conditionalAllow), func(t *testing.T) { + statement := `{"Effect":"Allow","Action":["s3:PutObjectRetention","s3:BypassGovernanceRetention"],"Resource":["arn:aws:s3:::` + bucket + `/*"]}, {"Effect":"Deny","Action":["s3:PutObjectRetention"],"Resource":["arn:aws:s3:::` + bucket + `/*"],"Condition":{"NumericLessThan":{"s3:object-lock-remaining-retention-days":"60"}}}` + if conditionalAllow { + statement = `{"Effect":"Allow","Action":["s3:BypassGovernanceRetention"],"Resource":["arn:aws:s3:::` + bucket + `/*"]}, {"Effect":"Allow","Action":["s3:PutObjectRetention"],"Resource":["arn:aws:s3:::` + bucket + `/*"],"Condition":{"NumericGreaterThanEquals":{"s3:object-lock-remaining-retention-days":"60"}}}` + } + cred := newRetentionPolicyUser(t, instance, `{"Version":"2012-10-17","Statement":[`+statement+`]}`) + key := fmt.Sprintf("review-minimum-%v", conditionalAllow) + far := UTCNow().Add(365 * 24 * time.Hour).Truncate(time.Second) + info, err := obj.PutObject(t.Context(), bucket, key, mustGetPutObjReader(t, bytes.NewReader(nil), 0, "", ""), ObjectOptions{ + Versioned: true, + UserDefined: map[string]string{ + strings.ToLower(xhttp.AmzObjectLockMode): "GOVERNANCE", + strings.ToLower(xhttp.AmzObjectLockRetainUntilDate): amztime.ISO8601Format(far), + }, + }) + if err != nil { + t.Fatal(err) + } + if status := putRetention(t, router, cred, bucket, key, info.VersionID, "GOVERNANCE", UTCNow().Add(30*24*time.Hour), true); status != http.StatusForbidden { + t.Fatalf("shortening to 30 days status = %d, want 403", status) + } + status := putRetention(t, router, cred, bucket, key, info.VersionID, "", time.Time{}, true) + after, err := obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: info.VersionID}) + if err != nil { + t.Fatal(err) + } + ret := objectlock.GetObjectRetentionMeta(after.UserDefined) + if status != http.StatusForbidden || ret.Mode != objectlock.RetGovernance { + t.Fatalf("clearing retention status = %d, stored retention = %s; want 403 and original GOVERNANCE", status, ret.String()) + } + }) + } + }, + }) +} + +type hookBeforeWrite struct { + ObjectLayer + key string + before func() +} + +func (p *hookBeforeWrite) fire(key string) { + if key == p.key && p.before != nil { + f := p.before + p.before = nil + f() + } +} + +func (p *hookBeforeWrite) PutObject(ctx context.Context, bucket, key string, r *PutObjReader, opts ObjectOptions) (ObjectInfo, error) { + p.fire(key) + return p.ObjectLayer.PutObject(ctx, bucket, key, r, opts) +} + +func (p *hookBeforeWrite) CopyObject(ctx context.Context, srcBucket, srcObject, dstBucket, dstObject string, srcInfo ObjectInfo, srcOpts, dstOpts ObjectOptions) (ObjectInfo, error) { + p.fire(dstObject) + return p.ObjectLayer.CopyObject(ctx, srcBucket, srcObject, dstBucket, dstObject, srcInfo, srcOpts, dstOpts) +} + +func (p *hookBeforeWrite) CompleteMultipartUpload(ctx context.Context, bucket, key, uploadID string, parts []CompletePart, opts ObjectOptions) (ObjectInfo, error) { + p.fire(key) + return p.ObjectLayer.CompleteMultipartUpload(ctx, bucket, key, uploadID, parts, opts) +} + +func TestDirectoryObjectLockAfterPreflight(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + makeBucketOptions: MakeBucketOptions{VersioningEnabled: true, LockEnabled: true}, + objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, root auth.Credentials, t *testing.T) { + writer := newObjectAttributesAuthzUser(t, instance, bucket, `"s3:GetObject","s3:PutObject"`) + srcDir := "other-dir/" + if rec := retentionAuthzRequest(t, router, writer, http.MethodPut, makeTestTargetURL("", bucket, srcDir, nil), nil, nil); rec.Code != http.StatusOK { + t.Fatalf("srcDir PUT: %d %s", rec.Code, rec.Body.String()) + } + + for _, mode := range []string{"GOVERNANCE", "COMPLIANCE", "LEGAL-HOLD"} { + for _, op := range []string{"PutObject", "CopyObject", "SelfCopy"} { + t.Run(instance+"/"+mode+"/"+op, func(t *testing.T) { + key := fmt.Sprintf("review-race-%s-%s/", mode, op) + if rec := retentionAuthzRequest(t, router, writer, http.MethodPut, makeTestTargetURL("", bucket, key, nil), nil, nil); rec.Code != http.StatusOK { + t.Fatalf("initial PUT: %d %s", rec.Code, rec.Body.String()) + } + + var locked ObjectInfo + hook := &hookBeforeWrite{ObjectLayer: obj, key: key} + hook.before = func() { + if mode == "LEGAL-HOLD" { + body := []byte(`ON`) + sum := md5.Sum(body) + rec := retentionAuthzRequest(t, router, root, http.MethodPut, makeTestTargetURL("", bucket, key, url.Values{"legal-hold": {""}, "versionId": {nullVersionID}}), body, map[string]string{xhttp.ContentMD5: base64.StdEncoding.EncodeToString(sum[:])}) + if rec.Code != http.StatusOK { + t.Fatalf("set legal hold: %d %s", rec.Code, rec.Body.String()) + } + } else if status := putRetention(t, router, root, bucket, key, nullVersionID, mode, UTCNow().Add(365*24*time.Hour), false); status != http.StatusOK { + t.Fatalf("set retention: %d", status) + } + var err error + locked, err = obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: nullVersionID}) + if err != nil { + t.Fatal(err) + } + } + setObjectLayer(hook) + defer setObjectLayer(obj) + + var rec *httptest.ResponseRecorder + switch op { + case "PutObject": + rec = retentionAuthzRequest(t, router, writer, http.MethodPut, makeTestTargetURL("", bucket, key, nil), nil, nil) + case "CopyObject": + rec = retentionAuthzRequest(t, router, writer, http.MethodPut, makeTestTargetURL("", bucket, key, nil), nil, map[string]string{ + xhttp.AmzCopySource: pathJoin(bucket, srcDir), + }) + case "SelfCopy": + rec = retentionAuthzRequest(t, router, writer, http.MethodPut, makeTestTargetURL("", bucket, key, nil), nil, map[string]string{ + xhttp.AmzCopySource: pathJoin(bucket, key), xhttp.AmzMetadataDirective: "REPLACE", "x-amz-meta-updated": "true", + }) + } + if hook.before != nil { + t.Fatalf("%s never reached backend: %d %s", op, rec.Code, rec.Body.String()) + } + after, err := obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: nullVersionID}) + if err != nil { + t.Fatal(err) + } + if rec.Code != http.StatusBadRequest { + t.Fatalf("locked null write via %s: %d %s", op, rec.Code, rec.Body.String()) + } + multipartConditionError(t, rec, "InvalidRequest") + retentionOverwriteUnchanged(t, locked, after) + }) + } + } + }, + }) +} + +func TestMultipartVersioningTransitions(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, root auth.Credentials, t *testing.T) { + t.Run(instance+"/unversioned-to-enabled", func(t *testing.T) { + key := "review-upload-before-versioning" + initRec := retentionAuthzRequest(t, router, root, http.MethodPost, makeTestTargetURL("", bucket, key, url.Values{"uploads": {""}}), nil, nil) + if initRec.Code != http.StatusOK { + t.Fatalf("init: %d %s", initRec.Code, initRec.Body.String()) + } + var mp InitiateMultipartUploadResponse + if err := xml.Unmarshal(initRec.Body.Bytes(), &mp); err != nil { + t.Fatal(err) + } + part := retentionAuthzRequest(t, router, root, http.MethodPut, getPutObjectPartURL("", bucket, key, mp.UploadID, "1"), []byte("ordinary pending upload"), nil) + if part.Code != http.StatusOK { + t.Fatalf("part: %d %s", part.Code, part.Body.String()) + } + if _, err := globalBucketMetadataSys.Update(t.Context(), bucket, bucketVersioningConfig, []byte(`Enabled`)); err != nil { + t.Fatal(err) + } + body := fmt.Appendf(nil, `1%s`, multipartConditionResponseETag(part)) + rec := retentionAuthzRequest(t, router, root, http.MethodPost, makeTestTargetURL("", bucket, key, url.Values{"uploadId": {mp.UploadID}}), body, nil) + if rec.Code != http.StatusOK { + t.Fatalf("ordinary upload initiated before versioning fails completion: %d %s", rec.Code, rec.Body.String()) + } + }) + t.Run(instance+"/addressed-upload-after-suspension", func(t *testing.T) { + key := "review-addressed-before-suspension" + original := []byte("original") + info, err := obj.PutObject(t.Context(), bucket, key, mustGetPutObjReader(t, bytes.NewReader(original), int64(len(original)), "", ""), ObjectOptions{Versioned: true}) + if err != nil { + t.Fatal(err) + } + up, err := obj.NewMultipartUpload(t.Context(), bucket, key, ObjectOptions{Versioned: true, VersionID: info.VersionID}) + if err != nil { + t.Fatal(err) + } + part := []byte("replaced existing version") + pi, err := obj.PutObjectPart(t.Context(), bucket, key, up.UploadID, 1, mustGetPutObjReader(t, bytes.NewReader(part), int64(len(part)), "", ""), ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + if _, err := globalBucketMetadataSys.Update(t.Context(), bucket, bucketVersioningConfig, []byte(`Suspended`)); err != nil { + t.Fatal(err) + } + writer := newObjectAttributesAuthzUser(t, instance, bucket, `"s3:PutObject"`) + body := fmt.Appendf(nil, `1"%s"`, pi.ETag) + rec := retentionAuthzRequest(t, router, writer, http.MethodPost, makeTestTargetURL("", bucket, key, url.Values{"uploadId": {up.UploadID}}), body, nil) + after, err := obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: info.VersionID}) + if err != nil { + t.Fatal(err) + } + if rec.Code != http.StatusForbidden || after.ETag != info.ETag { + t.Fatalf("suspended-bucket existing UUID overwritten: status=%d old-etag=%s new-etag=%s body=%s", rec.Code, info.ETag, after.ETag, rec.Body.String()) + } + }) + }, + }) +} diff --git a/cmd/object-retention-overwrite_test.go b/cmd/object-retention-overwrite_test.go new file mode 100644 index 000000000..640738737 --- /dev/null +++ b/cmd/object-retention-overwrite_test.go @@ -0,0 +1,302 @@ +// Copyright (c) 2026 Feng Ruohang +// SPDX-License-Identifier: AGPL-3.0-or-later +// +// This file is part of Silo Object Storage stack. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "bytes" + "context" + "crypto/md5" + "encoding/base64" + "errors" + "fmt" + "net/http" + "net/url" + "strings" + "testing" + "time" + + "github.com/minio/minio/internal/amztime" + "github.com/minio/minio/internal/auth" + objectlock "github.com/minio/minio/internal/bucket/object/lock" + xhttp "github.com/minio/minio/internal/http" +) + +func retentionOverwriteUnchanged(t *testing.T, before, after ObjectInfo) { + t.Helper() + if after.ETag != before.ETag || after.Size != before.Size || after.VersionID != before.VersionID || !after.ModTime.Equal(before.ModTime) { + t.Error("rejected write changed the stored object") + } + if objectlock.GetObjectRetentionMeta(after.UserDefined).String() != objectlock.GetObjectRetentionMeta(before.UserDefined).String() || objectlock.GetObjectLegalHoldMeta(after.UserDefined).Status != objectlock.GetObjectLegalHoldMeta(before.UserDefined).Status { + t.Error("rejected write changed the stored lock") + } +} + +func TestObjectLockOverwriteConfigUnavailable(t *testing.T) { + previous := newObjectLayerFn() + setObjectLayer(nil) + defer setObjectLayer(previous) + err := checkObjectLockedForOverwrite(t.Context(), "lock-config-unavailable", "object", func(context.Context, string, string, ObjectOptions) (ObjectInfo, error) { + t.Fatal("object must not be read without a bucket configuration") + return ObjectInfo{}, nil + }) + if !errors.Is(err, errServerNotInitialized) { + t.Fatalf("unavailable lock configuration must fail closed, got %v", err) + } +} + +func TestPutObjectRetentionMaximumDaysPolicy(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + makeBucketOptions: MakeBucketOptions{VersioningEnabled: true, LockEnabled: true}, + objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, _ auth.Credentials, t *testing.T) { + for _, conditionalAllow := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/conditional-allow=%t", instance, conditionalAllow), func(t *testing.T) { + statement := fmt.Sprintf(`{"Effect":"Allow","Action":["s3:PutObjectRetention"],"Resource":["arn:aws:s3:::%s/*"],"Condition":{"NumericLessThanEquals":{"s3:object-lock-remaining-retention-days":"200000"}}}`, bucket) + if !conditionalAllow { + statement = fmt.Sprintf(`{"Effect":"Allow","Action":["s3:PutObjectRetention"],"Resource":["arn:aws:s3:::%s/*"]},{"Effect":"Deny","Action":["s3:PutObjectRetention"],"Resource":["arn:aws:s3:::%s/*"],"Condition":{"NumericGreaterThan":{"s3:object-lock-remaining-retention-days":"200000"}}}`, bucket, bucket) + } + cred := newRetentionPolicyUser(t, instance, `{"Version":"2012-10-17","Statement":[`+statement+`]}`) + key := fmt.Sprintf("maximum-retention-%t", conditionalAllow) + initial, err := obj.PutObject(t.Context(), bucket, key, mustGetPutObjReader(t, bytes.NewReader(nil), 0, "", ""), ObjectOptions{Versioned: true}) + if err != nil { + t.Fatal(err) + } + // Both dates exceed time.Duration's range (~292 years). A saturated + // Time.Sub would report 106752 days and incorrectly allow 1000 years. + if status := putRetention(t, router, cred, bucket, key, initial.VersionID, "GOVERNANCE", UTCNow().AddDate(1000, 0, 0), false); status != http.StatusForbidden { + t.Fatalf("1000-year retention: want 403, got %d", status) + } + after, err := obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: initial.VersionID}) + if err != nil { + t.Fatal(err) + } + retentionOverwriteUnchanged(t, initial, after) + if status := putRetention(t, router, cred, bucket, key, initial.VersionID, "GOVERNANCE", UTCNow().AddDate(500, 0, 0), false); status != http.StatusOK { + t.Fatalf("500-year retention: want 200, got %d", status) + } + }) + } + }, + }) +} + +func TestAPICopyObjectPreservesVersionAfterSuspension(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + makeBucketOptions: MakeBucketOptions{VersioningEnabled: true}, + objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, root auth.Credentials, t *testing.T) { + t.Run(instance, func(t *testing.T) { + key := "self-copy-after-suspension" + original, err := obj.PutObject(t.Context(), bucket, key, mustGetPutObjReader(t, bytes.NewBufferString("payload"), 7, "", ""), ObjectOptions{ + Versioned: true, UserDefined: map[string]string{"x-amz-meta-state": "original"}, + }) + if err != nil { + t.Fatal(err) + } + if _, err := globalBucketMetadataSys.Update(t.Context(), bucket, bucketVersioningConfig, + []byte(`Suspended`)); err != nil { + t.Fatal(err) + } + rec := retentionAuthzRequest(t, router, root, http.MethodPut, makeTestTargetURL("", bucket, key, nil), nil, map[string]string{ + xhttp.AmzCopySource: "/" + bucket + "/" + key, xhttp.AmzMetadataDirective: "REPLACE", "x-amz-meta-state": "replacement", + }) + if rec.Code != http.StatusOK { + t.Fatalf("copy: %d %s", rec.Code, rec.Body.String()) + } + old, err := obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: original.VersionID}) + if err != nil { + t.Fatal(err) + } + if old.UserDefined["x-amz-meta-state"] != "original" { + t.Errorf("copy changed metadata of existing UUID %s: %v", original.VersionID, old.UserDefined) + } + current, err := obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: nullVersionID}) + if err != nil { + t.Fatalf("copy must create the null version: %v", err) + } + if current.UserDefined["X-Amz-Meta-State"] != "replacement" { + t.Errorf("new null version metadata: %v", current.UserDefined) + } + }) + }, + }) +} + +// A persisted null upload can survive an upgrade or a versioning transition. +// The completion must see locks committed after the handler's preflight. +func TestMultipartNullObjectLockAfterPreflight(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + makeBucketOptions: MakeBucketOptions{VersioningEnabled: true, LockEnabled: true}, + objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, root auth.Credentials, t *testing.T) { + writer := newObjectAttributesAuthzUser(t, instance, bucket, `"s3:PutObject"`) + for _, mode := range []string{"GOVERNANCE", "COMPLIANCE", "LEGAL-HOLD"} { + t.Run(instance+"/"+mode, func(t *testing.T) { + ctx := t.Context() + key := "multipart-null-" + mode + opts := ObjectOptions{Versioned: true, VersionID: nullVersionID, ReplicationRequest: true} + if _, err := obj.PutObject(ctx, bucket, key, mustGetPutObjReader(t, bytes.NewBufferString("original"), 8, "", ""), opts); err != nil { + t.Fatal(err) + } + mp, err := obj.NewMultipartUpload(ctx, bucket, key, opts) + if err != nil { + t.Fatal(err) + } + part, err := obj.PutObjectPart(ctx, bucket, key, mp.UploadID, 1, mustGetPutObjReader(t, bytes.NewBufferString("replacement"), 11, "", ""), ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + var locked ObjectInfo + hook := &hookBeforeWrite{ObjectLayer: obj, key: key, before: func() { + if mode == "LEGAL-HOLD" { + body := []byte(`ON`) + sum := md5.Sum(body) + rec := retentionAuthzRequest(t, router, root, http.MethodPut, makeTestTargetURL("", bucket, key, url.Values{"legal-hold": {""}, "versionId": {nullVersionID}}), body, map[string]string{xhttp.ContentMD5: base64.StdEncoding.EncodeToString(sum[:])}) + if rec.Code != http.StatusOK { + t.Fatalf("set legal hold: %d %s", rec.Code, rec.Body.String()) + } + } else if status := putRetention(t, router, root, bucket, key, nullVersionID, mode, UTCNow().Add(365*24*time.Hour), false); status != http.StatusOK { + t.Fatalf("set retention: %d", status) + } + var err error + locked, err = obj.GetObjectInfo(ctx, bucket, key, ObjectOptions{VersionID: nullVersionID}) + if err != nil { + t.Fatal(err) + } + }} + setObjectLayer(hook) + defer setObjectLayer(obj) + body := []byte(multipartConditionCompleteBody([]CompletePart{{PartNumber: 1, ETag: part.ETag}})) + rec := retentionAuthzRequest(t, router, writer, http.MethodPost, makeTestTargetURL("", bucket, key, url.Values{"uploadId": {mp.UploadID}}), body, nil) + if hook.before != nil || rec.Code != http.StatusBadRequest { + t.Fatalf("completion after locking: %d %s", rec.Code, rec.Body.String()) + } + multipartConditionError(t, rec, "InvalidRequest") + after, err := obj.GetObjectInfo(ctx, bucket, key, ObjectOptions{VersionID: nullVersionID}) + if err != nil { + t.Fatal(err) + } + retentionOverwriteUnchanged(t, locked, after) + parts, err := obj.ListObjectParts(ctx, bucket, key, mp.UploadID, 0, 100, ObjectOptions{}) + if err != nil || len(parts.Parts) != 1 || parts.Parts[0].ETag != part.ETag { + t.Fatalf("rejected completion removed upload parts: %v %+v", err, parts.Parts) + } + }) + } + }, + }) +} + +// The newer, unlocked copy is in the write pool; the lock exists only in the +// other pool. A set-local check would miss it even while holding a write lock. +func TestPoolsObjectLockOverwrite(t *testing.T) { + z, _ := consistencyPools(t) + bucket, _, err := initAPIHandlerTest(t.Context(), z, nil, MakeBucketOptions{VersioningEnabled: true, LockEnabled: true}) + if err != nil { + t.Fatal(err) + } + for _, state := range []string{"GOVERNANCE", "COMPLIANCE", "LEGAL-HOLD", "expired", "unlocked", "unreadable"} { + for _, op := range []string{"put", "copy", "self-copy", "multipart"} { + t.Run(state+"/"+op, func(t *testing.T) { + ctx := t.Context() + key := encodeDirObject("null-" + state + "-" + op + "/") + meta := map[string]string{} + switch state { + case "GOVERNANCE", "COMPLIANCE": + meta[strings.ToLower(xhttp.AmzObjectLockMode)] = state + meta[strings.ToLower(xhttp.AmzObjectLockRetainUntilDate)] = amztime.ISO8601Format(UTCNow().Add(365 * 24 * time.Hour)) + case "LEGAL-HOLD": + meta[strings.ToLower(xhttp.AmzObjectLockLegalHold)] = "ON" + case "expired": + meta[strings.ToLower(xhttp.AmzObjectLockMode)] = "GOVERNANCE" + meta[strings.ToLower(xhttp.AmzObjectLockRetainUntilDate)] = amztime.ISO8601Format(UTCNow().Add(-24 * time.Hour)) + } + seed := ObjectOptions{Versioned: true, VersionID: nullVersionID, ReplicationRequest: true, MTime: UTCNow().Add(-time.Hour), UserDefined: meta} + putConsistencyObject(t, z, bucket, key, 1, "original", seed) + seed.UserDefined = map[string]string{} + seed.MTime = UTCNow() + putConsistencyObject(t, z, bucket, key, 0, "original", seed) + before := make([]ObjectInfo, len(z.serverPools)) + for i, pool := range z.serverPools { + var err error + before[i], err = pool.GetObjectInfo(ctx, bucket, key, ObjectOptions{VersionID: nullVersionID}) + if err != nil { + t.Fatal(err) + } + } + defer conditionalPutPool(t, z, key, 0)() + var restore func() + if state == "unreadable" { + restore = conditionalPutSwapDisks(z.serverPools[1], key, func(d StorageAPI) StorageAPI { + return consistencyReadFaultDisk{StorageAPI: d, bucket: bucket, object: key} + }) + defer restore() + } + opts := ObjectOptions{Versioned: true, VersionID: nullVersionID, UserDefined: map[string]string{"x-amz-meta-updated": "true"}} + var writeErr error + switch op { + case "put": + _, writeErr = z.PutObject(ctx, bucket, key, mustGetPutObjReader(t, bytes.NewBufferString("replacement"), 11, "", ""), opts) + case "copy", "self-copy": + source := key + if op == "copy" { + source = "source-" + state + putConsistencyObject(t, z, bucket, source, 0, "replacement", ObjectOptions{Versioned: true}) + } + gr, gerr := z.GetObjectNInfo(ctx, bucket, source, nil, nil, ObjectOptions{}) + if gerr != nil { + t.Fatal(gerr) + } + defer gr.Close() + info := gr.ObjInfo + info.UserDefined = opts.UserDefined + info.metadataOnly = op == "self-copy" + info.PutObjReader = mustGetPutObjReader(t, gr, info.Size, "", "") + info.Reader = info.PutObjReader.Reader + _, writeErr = z.CopyObject(ctx, bucket, source, bucket, key, info, ObjectOptions{VersionID: nullVersionID}, opts) + case "multipart": + id, parts := multipartConditionUpload(t, z, bucket, key, 0, opts) + _, writeErr = z.CompleteMultipartUpload(ctx, bucket, key, id, parts, ObjectOptions{Versioned: true}) + } + allowed := state == "expired" || state == "unlocked" + if allowed { + if writeErr != nil { + t.Fatalf("unlocked write: %v", writeErr) + } + return + } + if state == "unreadable" { + if !isErrReadQuorum(writeErr) { + t.Fatalf("unreadable pool: want quorum error, got %v", writeErr) + } + restore() + } else if !errors.As(writeErr, &ObjectLocked{}) { + t.Fatalf("lock in other pool: want ObjectLocked, got %v", writeErr) + } + for i, original := range before { + after, gerr := z.serverPools[i].GetObjectInfo(ctx, bucket, key, ObjectOptions{VersionID: nullVersionID}) + if gerr != nil { + t.Fatal(gerr) + } + retentionOverwriteUnchanged(t, original, after) + } + }) + } + } +} diff --git a/cmd/replication-tagging-order_test.go b/cmd/replication-tagging-order_test.go index 2aa9f534c..1024646ff 100644 --- a/cmd/replication-tagging-order_test.go +++ b/cmd/replication-tagging-order_test.go @@ -238,7 +238,7 @@ func r5APIOrdering(t *testing.T, encrypted bool) { if err != nil { t.Fatal(err) } - original, err = obj.CompleteMultipartUpload(t.Context(), bucket, name, mp.UploadID, []CompletePart{{PartNumber: 1, ETag: part.ETag}}, ObjectOptions{Versioned: true}) + original, err = obj.CompleteMultipartUpload(t.Context(), bucket, name, mp.UploadID, []CompletePart{{PartNumber: 1, ETag: part.ETag}}, ObjectOptions{Versioned: true, ReplicationRequest: true}) if err != nil { t.Fatal(err) } From fa565ec9fc803fbfcf3a170ecc7651448fceddcc Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Wed, 14 Oct 2026 15:39:25 +0800 Subject: [PATCH 07/14] build: update Go to 1.27.2 and x/net to 0.60.0 Use the October security fixes for reachable standard-library and HTTP/2 vulnerabilities found by govulncheck. Align the module requirement, build dependency check and digest-pinned Docker build stages on Go 1.27.2. Update only golang.org/x/net and its checksums; module tidying and credit regeneration introduce no other dependency or license changes. Developed with Codex (Astra). Co-authored-by: Codex Signed-off-by: Feng Ruohang --- Dockerfile.goreleaser | 4 ++-- buildscripts/checkdeps.sh | 2 +- go.mod | 4 ++-- go.sum | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Dockerfile.goreleaser b/Dockerfile.goreleaser index 5311ef438..0d9944492 100644 --- a/Dockerfile.goreleaser +++ b/Dockerfile.goreleaser @@ -1,4 +1,4 @@ -FROM golang:1.27.1-alpine@sha256:cf6fca6641884b8433441b2b0652976f975e1d0fdd26d177eaaf8596087f3125 AS curl-build +FROM golang:1.27.2-alpine@sha256:f92b6ef800e499660581efdabdf25d9d817a9d124eaf900924f0504e7e27e12d AS curl-build ARG TARGETARCH COPY dockerscripts/build-static-curl.sh /build/build-static-curl RUN /bin/sh /build/build-static-curl @@ -9,7 +9,7 @@ COPY --from=curl-build /go/bin/curl /curl COPY --from=curl-build /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt ENTRYPOINT ["/curl"] -FROM golang:1.27.1-alpine@sha256:cf6fca6641884b8433441b2b0652976f975e1d0fdd26d177eaaf8596087f3125 AS build +FROM golang:1.27.2-alpine@sha256:f92b6ef800e499660581efdabdf25d9d817a9d124eaf900924f0504e7e27e12d AS build ARG TARGETARCH diff --git a/buildscripts/checkdeps.sh b/buildscripts/checkdeps.sh index 58b222462..db5ca1c87 100755 --- a/buildscripts/checkdeps.sh +++ b/buildscripts/checkdeps.sh @@ -7,7 +7,7 @@ _init() { ## Minimum required versions for build dependencies GIT_VERSION="1.0" - GO_VERSION="1.27.1" + GO_VERSION="1.27.2" OSX_VERSION="10.8" KNAME=$(uname -s) ARCH=$(uname -m) diff --git a/go.mod b/go.mod index ebe162bac..3a25fe190 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/minio/minio -go 1.27.1 +go 1.27.2 // Console and MC retain their historical module paths for best-effort upstream // compatibility. Pin the maintained PGSTY implementations used by SILO. @@ -294,7 +294,7 @@ require ( go.opentelemetry.io/otel/trace v1.45.0 // indirect go.uber.org/multierr v1.11.0 // indirect golang.org/x/mod v0.41.0 // indirect - golang.org/x/net v0.59.0 // indirect + golang.org/x/net v0.60.0 // indirect golang.org/x/text v0.42.0 // indirect golang.org/x/tools v0.50.0 // indirect google.golang.org/genproto v0.0.0-20260319201613-d00831a3d3e7 // indirect diff --git a/go.sum b/go.sum index a08f49d24..1b8f71b99 100644 --- a/go.sum +++ b/go.sum @@ -762,8 +762,8 @@ golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug golang.org/x/net v0.5.0/go.mod h1:DivGGAXEgPSlEBzxGzZI+ZLohi+xUj054jfeKui00ws= golang.org/x/net v0.6.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs= golang.org/x/net v0.7.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs= -golang.org/x/net v0.59.0 h1:5zfYln+w5XCxwrnMMJPufRgNoXEaGxl0wo5GqPXyues= -golang.org/x/net v0.59.0/go.mod h1:2DA/G1UfVbCpQPeWTmMPGY7Cs2PkBkwu743bVX5PIVg= +golang.org/x/net v0.60.0 h1:79p50tfZlm0J9YfoDsSi639qSXNGVwEzOPLCxM2FsYU= +golang.org/x/net v0.60.0/go.mod h1:2DA/G1UfVbCpQPeWTmMPGY7Cs2PkBkwu743bVX5PIVg= golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= From 38a5ab549c043bb0be75f61abef4accb1ab6648f Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Wed, 14 Oct 2026 15:48:51 +0800 Subject: [PATCH 08/14] build: use golangci-lint compatible with Go 1.27.2 Pin golangci-lint v2.14.0, whose x/tools v0.50.0 decoder supports the version 5 export data emitted by Go 1.27.2. The previous v2.13.1 binary failed to import internal/cpu in Linux CI before running the checks. Keep the existing linter selection and configuration. Developed with Codex (Astra). Co-authored-by: Codex Signed-off-by: Feng Ruohang --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index aaa97438b..5e80825da 100644 --- a/Makefile +++ b/Makefile @@ -4,7 +4,7 @@ LDFLAGS := $(shell go run buildscripts/gen-ldflags.go) GOOS ?= $(shell go env GOOS) GOARCH ?= $(shell go env GOARCH) -GOLANGCI_VERSION ?= v2.13.1 +GOLANGCI_VERSION ?= v2.14.0 VERSION ?= $(shell git describe --tags) REPO ?= docker.io/pgsty From 2d57b975d147bc2b6b42a796e488564ee32b3fc9 Mon Sep 17 00:00:00 2001 From: clown15 Date: Wed, 14 Oct 2026 08:55:51 +0000 Subject: [PATCH 09/14] cmd: fix fast-path modtime parsing in cleanupStaleUploadsOnDisk Upload ID directories on disk are named as 'x'. Attempting to decode them as Base64 fails unconditionally, causing the cleanup routine to bypass the timestamp check and fall back to disk.ReadVersion on every active upload. Directly parse the timestamp suffix from the plaintext directory name to restore O(1) stale upload detection, while preserving Base64 decoding and ReadVersion fallback for legacy uploads. Signed-off-by: clown15 --- cmd/erasure-multipart-cleanup_test.go | 84 +++++++++++++++++++++++++++ cmd/erasure-multipart.go | 28 +++++---- 2 files changed, 102 insertions(+), 10 deletions(-) create mode 100644 cmd/erasure-multipart-cleanup_test.go diff --git a/cmd/erasure-multipart-cleanup_test.go b/cmd/erasure-multipart-cleanup_test.go new file mode 100644 index 000000000..fa0fe92ba --- /dev/null +++ b/cmd/erasure-multipart-cleanup_test.go @@ -0,0 +1,84 @@ +// Copyright (c) 2026 clown15 +// +// This file is part of Silo Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "encoding/base64" + "fmt" + "testing" + "time" +) + +func TestExtractModTimeFromUploadIDDir(t *testing.T) { + now := time.Now().UTC().Truncate(time.Millisecond) + nano := now.UnixNano() + uuid := "d16ad42a-6fe5-4a3b-8092-69983142238f" + + testCases := []struct { + name string + dirName string + expectedMod time.Time + wantZero bool + }{ + { + name: "Modern Plaintext Format (x)", + dirName: fmt.Sprintf("%sx%d", uuid, nano), + expectedMod: time.Unix(0, nano), + wantZero: false, + }, + { + name: "Legacy Format Without Timestamp ()", + dirName: uuid, + expectedMod: time.Time{}, + wantZero: true, + }, + { + name: "Base64 Encoded Format", + dirName: base64.RawURLEncoding.EncodeToString([]byte(fmt.Sprintf("%sx%d", uuid, nano))), + expectedMod: time.Unix(0, nano), + wantZero: false, + }, + { + name: "Malformed Suffix (xinvalid)", + dirName: fmt.Sprintf("%sxnotanumber", uuid), + expectedMod: time.Time{}, + wantZero: true, + }, + { + name: "Empty Directory Name", + dirName: "", + expectedMod: time.Time{}, + wantZero: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := extractModTimeFromUploadIDDir(tc.dirName) + if tc.wantZero { + if !got.IsZero() { + t.Fatalf("expected zero time for %q, got %v", tc.dirName, got) + } + return + } + if !got.Equal(tc.expectedMod) { + t.Fatalf("extractModTimeFromUploadIDDir(%q) = %v; want %v", tc.dirName, got, tc.expectedMod) + } + }) + } +} diff --git a/cmd/erasure-multipart.go b/cmd/erasure-multipart.go index 89355c762..b48b62d78 100644 --- a/cmd/erasure-multipart.go +++ b/cmd/erasure-multipart.go @@ -174,6 +174,23 @@ func (er erasureObjects) deleteAll(ctx context.Context, bucket, prefix string) { wg.Wait() } +// extractModTimeFromUploadIDDir extracts creation time from the upload ID directory on disk. +// On-disk directory names are of the form x. +// For compatibility with legacy or encoded representations, if the directory is Base64 encoded, +// it decodes the name before parsing. +func extractModTimeFromUploadIDDir(uploadIDDir string) time.Time { + target := uploadIDDir + if b64, err := base64.RawURLEncoding.DecodeString(uploadIDDir); err == nil && strings.Contains(string(b64), "x") { + target = string(b64) + } + if split := strings.Split(target, "x"); len(split) == 2 { + if t, err := strconv.ParseInt(split[1], 10, 64); err == nil { + return time.Unix(0, t) + } + } + return time.Time{} +} + // Remove the old multipart uploads on the given disk. func (er erasureObjects) cleanupStaleUploadsOnDisk(ctx context.Context, disk StorageAPI) { drivePath := disk.Endpoint().Path @@ -181,16 +198,7 @@ func (er erasureObjects) cleanupStaleUploadsOnDisk(ctx context.Context, disk Sto readDirFn(pathJoin(drivePath, minioMetaMultipartBucket), func(shaDir string, typ os.FileMode) error { readDirFn(pathJoin(drivePath, minioMetaMultipartBucket, shaDir), func(uploadIDDir string, typ os.FileMode) error { uploadIDPath := pathJoin(shaDir, uploadIDDir) - var modTime time.Time - // Upload IDs are of the form base64_url(x), we can extract the time from the UUID. - if b64, err := base64.RawURLEncoding.DecodeString(uploadIDDir); err == nil { - if split := strings.Split(string(b64), "x"); len(split) == 2 { - t, err := strconv.ParseInt(split[1], 10, 64) - if err == nil { - modTime = time.Unix(0, t) - } - } - } + modTime := extractModTimeFromUploadIDDir(uploadIDDir) // Fallback for older uploads without time in the ID. if modTime.IsZero() { wait := deleteMultipartCleanupSleeper.Timer(ctx) From 71a1631f13835cc7f3bdceaa79027190393e60ef Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Wed, 14 Oct 2026 23:26:53 +0800 Subject: [PATCH 10/14] fix: reject object lock preflight read failures Return storage read failures before accepting an object overwrite. Keep the null-version and multipart target checks under the destination write lock, and add slow-body, read-fault, legacy-upload and recovery-collision regressions to the Object Lock race gate. Signed-off-by: Feng Ruohang --- .github/workflows/go.yml | 5 + cmd/bucket-object-lock.go | 7 +- cmd/object-retention-overwrite_test.go | 244 +++++++++++++++++++++++++ 3 files changed, 255 insertions(+), 1 deletion(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 860eebf83..8752cd267 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -102,6 +102,11 @@ jobs: - name: Run tag replication tests under race detector run: go test -race ./cmd -run '^TestAPITagging' -count=1 -timeout=5m + - name: Run Object Lock authorization and overwrite tests under race detector + env: + MINIO_API_REQUESTS_MAX: "10000" + run: go test -race -tags kqueue,dev ./cmd -run '^Test(APIPutObjectRetention|APIVersionAddressedWrite|PutObjectRetention|DirectoryObjectLock|ObjectLockOverwrite|APICopyObjectPreservesVersionAfterSuspension|MultipartNullObjectLock|PoolsObjectLockOverwrite|MultipartVersioningTransitions|PlanReview|RecoveredMultipartUpload)' -count=1 -timeout=5m + crosscompile: name: Cross Compile runs-on: ubuntu-latest diff --git a/cmd/bucket-object-lock.go b/cmd/bucket-object-lock.go index 80f65f615..397b039ef 100644 --- a/cmd/bucket-object-lock.go +++ b/cmd/bucket-object-lock.go @@ -332,7 +332,12 @@ func checkPutObjectLockAllowed(ctx context.Context, rq *http.Request, bucket, ob } if opts.VersionID != "" && !replicaTrusted { - if objInfo, err := getObjectInfoFn(ctx, bucket, object, opts); err == nil { + objInfo, err := getObjectInfoFn(ctx, bucket, object, opts) + if err != nil { + if !isErrObjectNotFound(err) && !isErrVersionNotFound(err) && !errors.As(err, &MethodNotAllowed{}) { + return mode, retainDate, legalHold, toAPIErrorCode(ctx, err) + } + } else { r := objectlock.GetObjectRetentionMeta(objInfo.UserDefined) t, err := objectlock.UTCNowNTP() if err != nil { diff --git a/cmd/object-retention-overwrite_test.go b/cmd/object-retention-overwrite_test.go index 640738737..b6a910cba 100644 --- a/cmd/object-retention-overwrite_test.go +++ b/cmd/object-retention-overwrite_test.go @@ -25,9 +25,12 @@ import ( "encoding/base64" "errors" "fmt" + "io" "net/http" + "net/http/httptest" "net/url" "strings" + "sync" "testing" "time" @@ -300,3 +303,244 @@ func TestPoolsObjectLockOverwrite(t *testing.T) { } } } + +type planReviewGatedBody struct { + io.ReadCloser + first sync.Once + ready chan struct{} + release chan struct{} +} + +func (g *planReviewGatedBody) Read(p []byte) (int, error) { + g.first.Do(func() { close(g.ready); <-g.release }) + return g.ReadCloser.Read(p) +} + +func TestPlanReviewSlowDirectoryPut(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + makeBucketOptions: MakeBucketOptions{VersioningEnabled: true, LockEnabled: true}, + objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, root auth.Credentials, t *testing.T) { + writer := newObjectAttributesAuthzUser(t, instance, bucket, `"s3:PutObject"`) + t.Run(instance, func(t *testing.T) { + key := "slow-body-directory/" + before, err := obj.PutObject(t.Context(), bucket, key, mustGetPutObjReader(t, bytes.NewBufferString("original"), 8, "", ""), ObjectOptions{Versioned: true, VersionID: nullVersionID}) + if err != nil { + t.Fatal(err) + } + payload := []byte("replacement-body") + req, err := newTestSignedRequestV4(http.MethodPut, makeTestTargetURL("", bucket, key, nil), int64(len(payload)), bytes.NewReader(payload), writer.AccessKey, writer.SecretKey, nil) + if err != nil { + t.Fatal(err) + } + gate := &planReviewGatedBody{ReadCloser: req.Body, ready: make(chan struct{}), release: make(chan struct{})} + req.Body = gate + var once sync.Once + release := func() { once.Do(func() { close(gate.release) }) } + defer release() + rec := httptest.NewRecorder() + done := make(chan struct{}) + go func() { router.ServeHTTP(rec, req); close(done) }() + select { + case <-gate.ready: + case <-done: + t.Fatalf("PUT finished before body: %d %s", rec.Code, rec.Body.String()) + case <-time.After(5 * time.Second): + t.Fatal("PUT did not reach body read") + } + locked := make(chan int, 1) + go func() { + locked <- putRetention(t, router, root, bucket, key, nullVersionID, "COMPLIANCE", UTCNow().Add(365*24*time.Hour), false) + }() + select { + case status := <-locked: + if status != http.StatusOK { + t.Fatalf("set lock while body paused: %d", status) + } + case <-time.After(5 * time.Second): + t.Fatal("retention update blocked behind paused body") + } + release() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("PUT did not finish") + } + after, err := obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: nullVersionID}) + if err != nil { + t.Fatal(err) + } + if rec.Code != http.StatusBadRequest || after.ETag != before.ETag || objectlock.GetObjectRetentionMeta(after.UserDefined).Mode != objectlock.RetCompliance { + t.Fatal("paused ordinary PUT overwrote the COMPLIANCE null version after the lock was acknowledged") + } + }) + }, + }) +} + +type planReviewReadFault struct { + ObjectLayer + key string + fired bool + putCalled bool +} + +func (p *planReviewReadFault) GetObjectInfo(ctx context.Context, bucket, key string, opts ObjectOptions) (ObjectInfo, error) { + if key == p.key && opts.VersionID == nullVersionID && !p.fired { + p.fired = true + return ObjectInfo{}, InsufficientReadQuorum{} + } + return p.ObjectLayer.GetObjectInfo(ctx, bucket, key, opts) +} + +func (p *planReviewReadFault) PutObject(ctx context.Context, bucket, key string, data *PutObjReader, opts ObjectOptions) (ObjectInfo, error) { + if key == p.key { + p.putCalled = true + } + return p.ObjectLayer.PutObject(ctx, bucket, key, data, opts) +} + +func TestPlanReviewPreflightReadFailure(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + makeBucketOptions: MakeBucketOptions{VersioningEnabled: true, LockEnabled: true}, + objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, _ auth.Credentials, t *testing.T) { + writer := newObjectAttributesAuthzUser(t, instance, bucket, `"s3:PutObject"`) + t.Run(instance, func(t *testing.T) { + key := "locked-directory-read-fault/" + _, err := obj.PutObject(t.Context(), bucket, key, mustGetPutObjReader(t, bytes.NewBufferString("original"), 8, "", ""), ObjectOptions{ + Versioned: true, VersionID: nullVersionID, ReplicationRequest: true, + UserDefined: map[string]string{ + strings.ToLower(xhttp.AmzObjectLockMode): "COMPLIANCE", + strings.ToLower(xhttp.AmzObjectLockRetainUntilDate): amztime.ISO8601Format(UTCNow().Add(365 * 24 * time.Hour)), + }, + }) + if err != nil { + t.Fatal(err) + } + initial, err := obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: nullVersionID}) + if err != nil { + t.Fatal(err) + } + fault := &planReviewReadFault{ObjectLayer: obj, key: key} + setObjectLayer(fault) + defer setObjectLayer(obj) + rec := retentionAuthzRequest(t, router, writer, http.MethodPut, makeTestTargetURL("", bucket, key, nil), []byte("replacement"), nil) + if !fault.fired { + t.Fatal("preflight fault was not injected") + } + if fault.putCalled { + t.Fatal("preflight read error allowed PutObject to proceed to storage layer") + } + after, err := obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: nullVersionID}) + if err != nil { + t.Fatal(err) + } + if rec.Code < 400 { + t.Errorf("metadata read quorum error was treated as absence: status=%d", rec.Code) + } + retentionOverwriteUnchanged(t, initial, after) + }) + }, + }) +} + +func TestPlanReviewLegacyMPUCompliance(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + makeBucketOptions: MakeBucketOptions{VersioningEnabled: true, LockEnabled: true}, + objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, root auth.Credentials, t *testing.T) { + writer := newObjectAttributesAuthzUser(t, instance, bucket, `"s3:PutObject"`) + t.Run(instance, func(t *testing.T) { + key := "legacy-mpu-compliance" + initial, err := obj.PutObject(t.Context(), bucket, key, mustGetPutObjReader(t, bytes.NewBufferString("original"), 8, "", ""), ObjectOptions{Versioned: true}) + if err != nil { + t.Fatal(err) + } + up, err := obj.NewMultipartUpload(t.Context(), bucket, key, ObjectOptions{Versioned: true, VersionID: initial.VersionID}) + if err != nil { + t.Fatal(err) + } + payload := []byte("replacement-part") + part, err := obj.PutObjectPart(t.Context(), bucket, key, up.UploadID, 1, mustGetPutObjReader(t, bytes.NewReader(payload), int64(len(payload)), "", ""), ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + if status := putRetention(t, router, root, bucket, key, initial.VersionID, "COMPLIANCE", UTCNow().Add(365*24*time.Hour), false); status != http.StatusOK { + t.Fatalf("set compliance: %d", status) + } + before, err := obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: initial.VersionID}) + if err != nil { + t.Fatal(err) + } + body := fmt.Appendf(nil, `1"%s"`, part.ETag) + rec := retentionAuthzRequest(t, router, writer, http.MethodPost, makeTestTargetURL("", bucket, key, url.Values{"uploadId": {up.UploadID}}), body, nil) + after, err := obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: initial.VersionID}) + if err != nil { + t.Fatal(err) + } + if rec.Code != http.StatusForbidden { + t.Errorf("legacy upload replaced locked UUID: status=%d", rec.Code) + } + retentionOverwriteUnchanged(t, before, after) + }) + }, + }) +} + +// A recovered upload retains an authentic server-generated version ID while a +// surviving replica reintroduces its already-completed version and lock. +// Only the fixture uses replication privileges; completion uses PutObject only. +func TestRecoveredMultipartUploadAfterReplica(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + makeBucketOptions: MakeBucketOptions{VersioningEnabled: true, LockEnabled: true}, + objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, root auth.Credentials, t *testing.T) { + writer := newObjectAttributesAuthzUser(t, instance, bucket, `"s3:PutObject"`) + t.Run(instance, func(t *testing.T) { + z := obj.(*erasureServerPools) + key := "recovered-generated-upload" + up, err := obj.NewMultipartUpload(t.Context(), bucket, key, ObjectOptions{Versioned: true}) + if err != nil { + t.Fatal(err) + } + er := z.serverPools[0].getHashedSet(key) + fi, _, err := er.checkUploadIDExists(t.Context(), bucket, key, up.UploadID, true) + if err != nil { + t.Fatal(err) + } + _, err = obj.PutObject(t.Context(), bucket, key, + mustGetPutObjReader(t, bytes.NewBufferString("protected-original"), 18, "", ""), + ObjectOptions{Versioned: true, VersionID: fi.VersionID, ReplicationRequest: true}) + if err != nil { + t.Fatal(err) + } + if status := putRetention(t, router, root, bucket, key, fi.VersionID, "COMPLIANCE", UTCNow().Add(365*24*time.Hour), false); status != http.StatusOK { + t.Fatalf("setting compliance lock: %d", status) + } + before, err := obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: fi.VersionID}) + if err != nil { + t.Fatal(err) + } + partRec := retentionAuthzRequest(t, router, writer, http.MethodPut, + makeTestTargetURL("", bucket, key, url.Values{"uploadId": {up.UploadID}, "partNumber": {"1"}}), + []byte("attacker-replacement"), nil) + if partRec.Code != http.StatusOK { + t.Fatalf("upload part: %d %s", partRec.Code, partRec.Body.String()) + } + etag := multipartConditionResponseETag(partRec) + body := []byte(multipartConditionCompleteBody([]CompletePart{{PartNumber: 1, ETag: etag}})) + rec := retentionAuthzRequest(t, router, writer, http.MethodPost, + makeTestTargetURL("", bucket, key, url.Values{"uploadId": {up.UploadID}}), body, nil) + after, err := obj.GetObjectInfo(t.Context(), bucket, key, ObjectOptions{VersionID: fi.VersionID}) + if err != nil { + t.Fatal(err) + } + if rec.Code != http.StatusForbidden { + t.Errorf("recovered upload bypassed existing UUID guard: status=%d", rec.Code) + } + retentionOverwriteUnchanged(t, before, after) + }) + }, + }) +} From 86c5a92eef3d5667c83bdbc59a033b955bdbf146 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Thu, 15 Oct 2026 09:48:16 +0800 Subject: [PATCH 11/14] fix: keep scanner response resets local and strengthen regression tests Keep the legacy header resets and clear the unused NSScanner response field at each construction site. Remove the generic stream Recycler call: Recycler transfers resource ownership and cannot be treated as a reset hook followed by another insertion into the handler's pool. Exercise the actual NSScanner handler across repeated scans with dirty response allocations, so the regression test does not depend on sync.Pool reusing an object. Check both response variants after wire serialization. Cover legacy DecodeMsg and reused indexed metadata headers as well. Retain the original contributor's commits and DCO sign-offs. Signed-off-by: Feng Ruohang --- cmd/storage-rest-common.go | 10 ---- cmd/storage-rest-server_test.go | 88 ++++++++++++++++++++++++++------ cmd/xl-storage-format-v2_test.go | 38 ++++++++++---- internal/grid/handlers.go | 5 -- 4 files changed, 102 insertions(+), 39 deletions(-) diff --git a/cmd/storage-rest-common.go b/cmd/storage-rest-common.go index 17bcc5f69..d8a28ae29 100644 --- a/cmd/storage-rest-common.go +++ b/cmd/storage-rest-common.go @@ -83,13 +83,3 @@ type nsScannerResp struct { Update *dataUsageEntry `msg:"u"` Final *dataUsageCache `msg:"f"` } - -// Recycle resets the response, so that recycled responses cannot -// leak stale scanner data into a subsequent scan. -func (r *nsScannerResp) Recycle() { - if r == nil { - return - } - r.Update = nil - r.Final = nil -} diff --git a/cmd/storage-rest-server_test.go b/cmd/storage-rest-server_test.go index 354348882..93ce35803 100644 --- a/cmd/storage-rest-server_test.go +++ b/cmd/storage-rest-server_test.go @@ -1,6 +1,6 @@ -// Copyright (c) 2015-2023 MinIO, Inc. +// Copyright (c) 2026 Julien Laurenceau and Feng Ruohang // -// This file is part of MinIO Object Storage stack +// This file is part of Silo Object Storage. // // This program is free software: you can redistribute it and/or modify // it under the terms of the GNU Affero General Public License as published by @@ -17,20 +17,78 @@ package cmd -import "testing" +import ( + "context" + "testing" -// TestNSScannerRespPooling verifies that recycled NSScanner stream -// responses cannot leak stale fields from a previous scan into new -// responses, which would otherwise produce incorrect bucket usage -// data. -func TestNSScannerRespPooling(t *testing.T) { - dirty := storageNSScannerRPC.NewResponse() - dirty.Update = &dataUsageEntry{} - dirty.Final = &dataUsageCache{} - storageNSScannerRPC.PutResponse(dirty) + "github.com/minio/madmin-go/v3" + "github.com/minio/minio/internal/grid" +) - resp := storageNSScannerRPC.NewResponse() - if resp.Update != nil || resp.Final != nil { - t.Fatalf("pooled nsScannerResp leaked stale fields: Update %v, Final %v", resp.Update, resp.Final) +type scannerResponseTestDisk struct { + StorageAPI +} + +func (scannerResponseTestDisk) NSScanner(_ context.Context, cache dataUsageCache, updates chan<- dataUsageEntry, _ madmin.HealScanMode, _ func() bool) (dataUsageCache, error) { + defer close(updates) + updates <- dataUsageEntry{Size: int64(cache.Info.NextCycle)} + cache.Info.NextCycle++ + return cache, nil +} + +func TestNSScannerHandlerClearsStaleResponses(t *testing.T) { + endpoint, err := NewEndpoint(t.TempDir()) + if err != nil { + t.Fatal(err) + } + globalLocalDrivesMu.Lock() + oldDrives, oldSets := globalLocalDrivesMap, globalLocalSetDrives + globalLocalDrivesMap = map[string]StorageAPI{endpoint.String(): scannerResponseTestDisk{}} + globalLocalSetDrives = nil + globalLocalDrivesMu.Unlock() + t.Cleanup(func() { + globalLocalDrivesMu.Lock() + globalLocalDrivesMap, globalLocalSetDrives = oldDrives, oldSets + globalLocalDrivesMu.Unlock() + }) + + oldRPC := storageNSScannerRPC + t.Cleanup(func() { storageNSScannerRPC = oldRPC }) + // Every new allocation is dirty too: the check must not depend on + // sync.Pool returning a previously used object. + storageNSScannerRPC = grid.NewStream[*nsScannerOptions, grid.NoPayload, *nsScannerResp](grid.HandlerNSScanner, + func() *nsScannerOptions { return &nsScannerOptions{} }, nil, + func() *nsScannerResp { + return &nsScannerResp{Update: &dataUsageEntry{Size: -1}, Final: &dataUsageCache{}} + }) + server := storageRESTServer{endpoint: endpoint} + for cycle := uint32(1); cycle <= 3; cycle++ { + out := make(chan *nsScannerResp, 2) + params := &nsScannerOptions{Cache: &dataUsageCache{Info: dataUsageCacheInfo{Name: "bucket", NextCycle: cycle}}} + if err := server.NSScannerHandler(t.Context(), params, out); err != nil { + t.Fatal(err) + } + if len(out) != 2 { + t.Fatalf("cycle %d: got %d responses, want an update and a final cache", cycle, len(out)) + } + for i := 0; i < 2; i++ { + resp := <-out + wire, err := resp.MarshalMsg(nil) + storageNSScannerRPC.PutResponse(resp) + if err != nil { + t.Fatal(err) + } + var decoded nsScannerResp + if _, err := decoded.UnmarshalMsg(wire); err != nil { + t.Fatal(err) + } + if i == 0 { + if decoded.Update == nil || decoded.Final != nil || decoded.Update.Size != int64(cycle) { + t.Errorf("cycle %d: invalid update response: %+v", cycle, decoded) + } + } else if decoded.Update != nil || decoded.Final == nil || decoded.Final.Info.NextCycle != cycle+1 { + t.Errorf("cycle %d: invalid final response: %+v", cycle, decoded) + } + } } } diff --git a/cmd/xl-storage-format-v2_test.go b/cmd/xl-storage-format-v2_test.go index 0076c8046..f5f682500 100644 --- a/cmd/xl-storage-format-v2_test.go +++ b/cmd/xl-storage-format-v2_test.go @@ -1214,14 +1214,34 @@ func TestXLMetaV2HeaderLegacyUnmarshalResetsEc(t *testing.T) { legacy = msgp.AppendUint8(legacy, uint8(base.Type)) legacy = msgp.AppendUint8(legacy, uint8(base.Flags)) - header := base - header.EcN, header.EcM = 4, 8 - x2 := xlMetaV2VersionHeaderV2{xlMetaV2VersionHeader: &header} - if _, err := x2.UnmarshalMsg(legacy); err != nil { - t.Fatalf("legacy V2 header unmarshal failed: %v", err) - } - if header.EcN != 0 || header.EcM != 0 { - t.Fatalf("stale erasure metadata leaked across objects: EcN %d, EcM %d", header.EcN, header.EcM) + for _, decoder := range []string{"UnmarshalMsg", "DecodeMsg", "loadIndexed"} { + t.Run(decoder, func(t *testing.T) { + header := base + header.EcN, header.EcM = 4, 8 + x2 := xlMetaV2VersionHeaderV2{xlMetaV2VersionHeader: &header} + var err error + switch decoder { + case "UnmarshalMsg": + _, err = x2.UnmarshalMsg(legacy) + case "DecodeMsg": + err = x2.DecodeMsg(msgp.NewReader(bytes.NewReader(legacy))) + case "loadIndexed": + indexed := msgp.AppendUint(nil, 2) + indexed = msgp.AppendUint(indexed, xlMetaVersion) + indexed = msgp.AppendInt(indexed, 1) + indexed = msgp.AppendBytes(indexed, legacy) + indexed = msgp.AppendBytes(indexed, nil) + xl := xlMetaV2{versions: []xlMetaV2ShallowVersion{{header: header}}} + err = xl.loadIndexed(indexed, nil) + header = xl.versions[0].header + } + if err != nil { + t.Fatal(err) + } + if header != base { + t.Fatalf("legacy header: got %+v, want %+v", header, base) + } + }) } // V1 headers (4 fields, never released) must zero the fields too. @@ -1232,7 +1252,7 @@ func TestXLMetaV2HeaderLegacyUnmarshalResetsEc(t *testing.T) { v1 = msgp.AppendUint8(v1, uint8(base.Type)) v1 = msgp.AppendUint8(v1, uint8(base.Flags)) - header = base + header := base header.EcN, header.EcM = 4, 8 header.Signature = [4]byte{9, 9, 9, 9} if _, err := header.unmarshalV1(v1); err != nil { diff --git a/internal/grid/handlers.go b/internal/grid/handlers.go index b0ca9e324..1d20fa9b9 100644 --- a/internal/grid/handlers.go +++ b/internal/grid/handlers.go @@ -698,11 +698,6 @@ func (h *StreamTypeHandler[Payload, Req, Resp]) PutRequest(r Req) { // These should be returned by the caller. func (h *StreamTypeHandler[Payload, Req, Resp]) PutResponse(r Resp) { if r != h.nilResp { - if rc, ok := any(r).(Recycler); ok { - // Recycled stream responses must not leak fields - // from a previous use into the next response. - rc.Recycle() - } //nolint:staticcheck // SA6002 IT IS A GENERIC VALUE! (and always a pointer) h.respPool.Put(r) } From 7a3ca0f82100cce06289a465f54139da2639f642 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Thu, 15 Oct 2026 13:20:15 +0800 Subject: [PATCH 12/14] build: gate releases on Object Lock regressions (#237) Run the existing Object Lock authorization and overwrite race regressions before release state changes, builds, packages, and uploads. Signed-off-by: Feng Ruohang --- .github/workflows/release.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index ead08d4fd..f490a9ec1 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -70,6 +70,12 @@ jobs: buildscripts/verify-rebrand.sh dockerscripts/docker-entrypoint_test.sh + # Keep this gate aligned with the Object Lock race regression in go.yml. + - name: Gate release on Object Lock authorization and overwrite tests under race detector + env: + MINIO_API_REQUESTS_MAX: "10000" + run: go test -race -tags kqueue,dev ./cmd -run '^Test(APIPutObjectRetention|APIVersionAddressedWrite|PutObjectRetention|DirectoryObjectLock|ObjectLockOverwrite|APICopyObjectPreservesVersionAfterSuspension|MultipartNullObjectLock|PoolsObjectLockOverwrite|MultipartVersioningTransitions|PlanReview|RecoveredMultipartUpload)' -count=1 -timeout=5m + - name: Compute release variables env: # Passed through the environment, never interpolated into the script From eeff112491f0cd2454af91833b4475c3106fe079 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Thu, 15 Oct 2026 18:18:45 +0800 Subject: [PATCH 13/14] test: verify commit metadata loss and write quorum boundaries Signed-off-by: Feng Ruohang --- cmd/erasure-metadata-utils_test.go | 62 ++++++++++++------- cmd/xl-storage-errors_test.go | 97 ++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 22 deletions(-) diff --git a/cmd/erasure-metadata-utils_test.go b/cmd/erasure-metadata-utils_test.go index 917d8249b..559b892bf 100644 --- a/cmd/erasure-metadata-utils_test.go +++ b/cmd/erasure-metadata-utils_test.go @@ -144,14 +144,16 @@ func TestReduceWriteQuorumErrsCommitMetaMissing(t *testing.T) { // from RenameData, enough to drive renameData() in unit tests. type renameDataFailingDisk struct { StorageAPI - err error + err error + undoWrite bool } func (d *renameDataFailingDisk) RenameData(context.Context, string, string, FileInfo, string, string, RenameOptions) (RenameDataResp, error) { return RenameDataResp{}, d.err } -func (d *renameDataFailingDisk) DeleteVersion(context.Context, string, string, FileInfo, bool, DeleteOptions) error { +func (d *renameDataFailingDisk) DeleteVersion(_ context.Context, _, _ string, _ FileInfo, _ bool, opts DeleteOptions) error { + d.undoWrite = opts.UndoWrite return nil } @@ -160,26 +162,42 @@ func (d *renameDataFailingDisk) DeleteVersion(context.Context, string, string, F // while an in-quorum errFileNotFound keeps its meaning so that // PutObject() still reports IncompleteBody for client disconnects. func TestRenameDataCommitMetaMissing(t *testing.T) { - metadata := make([]FileInfo, 16) - for i := range metadata { - metadata[i] = FileInfo{Deleted: true} // valid without erasure info - } - - disks := make([]StorageAPI, 16) - for i := range disks { - disks[i] = &renameDataFailingDisk{err: errCommitMetaMissing} - } - _, _, _, err := renameData(t.Context(), disks, minioMetaTmpBucket, "tmp", metadata, "bucket", "object", 9) - if !errors.Is(err, errErasureWriteQuorum) { - t.Fatalf("expected %s for in-quorum commit failure, got %v", errErasureWriteQuorum, err) - } - - for i := range disks { - disks[i] = &renameDataFailingDisk{err: errFileNotFound} - } - _, _, _, err = renameData(t.Context(), disks, minioMetaTmpBucket, "tmp", metadata, "bucket", "object", 9) - if !errors.Is(err, errFileNotFound) { - t.Fatalf("expected %s for in-quorum staging failure, got %v", errFileNotFound, err) + for _, tc := range []struct { + name string + failures int + diskErr error + wantErr error + }{ + {"all_commit_metadata_missing", 16, errCommitMetaMissing, errErasureWriteQuorum}, + {"quorum_commit_metadata_missing", 9, errCommitMetaMissing, errErasureWriteQuorum}, + {"neither_side_has_quorum", 8, errCommitMetaMissing, errErasureWriteQuorum}, + {"successful_write_quorum", 7, errCommitMetaMissing, nil}, + {"staging_failure", 16, errFileNotFound, errFileNotFound}, + } { + t.Run(tc.name, func(t *testing.T) { + metadata := make([]FileInfo, 16) + disks := make([]StorageAPI, 16) + for i := range disks { + metadata[i] = FileInfo{Deleted: true} // valid without erasure info + disk := &renameDataFailingDisk{} + if i < tc.failures { + disk.err = tc.diskErr + } + disks[i] = disk + } + _, _, _, err := renameData(t.Context(), disks, minioMetaTmpBucket, "tmp", metadata, "bucket", "object", 9) + if !errors.Is(err, tc.wantErr) { + t.Fatalf("expected %v, got %v", tc.wantErr, err) + } + for i, disk := range disks { + // Failed writes undo only the drives that committed. A + // successful quorum must never be rolled back. + wantUndo := tc.wantErr != nil && i >= tc.failures + if got := disk.(*renameDataFailingDisk).undoWrite; got != wantUndo { + t.Errorf("disk %d: UndoWrite = %v, want %v", i, got, wantUndo) + } + } + }) } } diff --git a/cmd/xl-storage-errors_test.go b/cmd/xl-storage-errors_test.go index 0551fed6a..b9d12fc4d 100644 --- a/cmd/xl-storage-errors_test.go +++ b/cmd/xl-storage-errors_test.go @@ -18,11 +18,16 @@ package cmd import ( + "context" "errors" + "net/http" "os" + "path/filepath" "runtime" + "sync" "syscall" "testing" + "time" ) func TestSysErrors(t *testing.T) { @@ -81,4 +86,96 @@ func TestCommitMetaMissingMapping(t *testing.T) { if qerr.Bucket != "bucket" || qerr.Object != "object" { t.Fatalf("unexpected bucket/object %s/%s", qerr.Bucket, qerr.Object) } + apiErr := toAPIError(t.Context(), err) + if apiErr.Code != "SlowDownWrite" || apiErr.HTTPStatusCode != http.StatusServiceUnavailable { + t.Fatalf("expected SlowDownWrite/503, got %+v", apiErr) + } +} + +// missingCommitMetaContext removes staged metadata at an existing cancellation +// check, after WriteAll and (for non-inline objects) the data rename succeeded. +// This deterministically models concurrent cleanup without changing production +// code or relying on a filesystem race. The context itself remains live. +type missingCommitMetaContext struct { + context.Context + mu sync.Mutex + meta, dataDest string + removed bool + removeErr error +} + +func (c *missingCommitMetaContext) Done() <-chan struct{} { + c.mu.Lock() + defer c.mu.Unlock() + if !c.removed { + if _, err := os.Stat(c.meta); err == nil { + dataMoved := c.dataDest == "" + if !dataMoved { + _, err = os.Stat(c.dataDest) + dataMoved = err == nil + } + if dataMoved { + c.removeErr = os.Remove(c.meta) + c.removed = c.removeErr == nil + } + } + } + return c.Context.Done() +} + +func TestXLStorageRenameDataCommitMetaMissing(t *testing.T) { + for _, tc := range []struct { + name string + dataFile bool + }{ + {"empty_object", false}, + {"data_directory", true}, + } { + t.Run(tc.name, func(t *testing.T) { + disk, root, err := newXLStorageTestSetup(t) + if err != nil { + t.Fatal(err) + } + defer disk.Close() + s := disk.storage + if err := s.MakeVol(t.Context(), "bucket"); err != nil { + t.Fatal(err) + } + fi := newFileInfo("object", 2, 2) + fi.DataDir, fi.VersionID = mustGetUUID(), mustGetUUID() + fi.Erasure.Index = 1 + fi.ModTime = time.Now().UTC() + fi.Fresh = true + ctx := &missingCommitMetaContext{ + Context: t.Context(), + meta: filepath.Join(root, minioMetaTmpBucket, "staged", xlStorageFormatFile), + } + if tc.dataFile { + fi.Size = 1 + fi.AddObjectPart(1, "", 1, 1, fi.ModTime, nil, nil) + part := filepath.Join(root, minioMetaTmpBucket, "staged", fi.DataDir, "part.1") + if err := os.MkdirAll(filepath.Dir(part), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(part, []byte("a"), 0o644); err != nil { + t.Fatal(err) + } + ctx.dataDest = filepath.Join(root, "bucket", "object", fi.DataDir) + } + _, err = s.RenameData(ctx, minioMetaTmpBucket, "staged", fi, "bucket", "object", RenameOptions{}) + if !ctx.removed { + t.Fatalf("metadata removal did not execute: removal error = %v, RenameData error = %v", ctx.removeErr, err) + } + if !errors.Is(err, errCommitMetaMissing) { + t.Fatalf("expected commit metadata missing, got %v", err) + } + apiErr := toAPIError(t.Context(), toObjectErr(err, "bucket", "object")) + if apiErr.Code != "SlowDownWrite" || apiErr.HTTPStatusCode != http.StatusServiceUnavailable { + t.Fatalf("expected SlowDownWrite/503, got %+v", apiErr) + } + if _, err := os.Stat(filepath.Join(root, "bucket", "object", xlStorageFormatFile)); !os.IsNotExist(err) { + t.Fatalf("failed commit must not produce destination metadata: %v", err) + } + }) + } } From bfc7906064a2338e7c944873780af912356dda33 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Thu, 15 Oct 2026 18:19:37 +0800 Subject: [PATCH 14/14] fix: prefer native multipart timestamps during cleanup Signed-off-by: Feng Ruohang --- cmd/erasure-multipart-cleanup_test.go | 95 ++++++++++++++++++++++++++- cmd/erasure-multipart.go | 17 ++--- 2 files changed, 103 insertions(+), 9 deletions(-) diff --git a/cmd/erasure-multipart-cleanup_test.go b/cmd/erasure-multipart-cleanup_test.go index fa0fe92ba..3181a4b1e 100644 --- a/cmd/erasure-multipart-cleanup_test.go +++ b/cmd/erasure-multipart-cleanup_test.go @@ -18,14 +18,18 @@ package cmd import ( + "context" "encoding/base64" "fmt" + "net/url" + "os" + "path/filepath" "testing" "time" ) func TestExtractModTimeFromUploadIDDir(t *testing.T) { - now := time.Now().UTC().Truncate(time.Millisecond) + now := time.Unix(1792056747, 123456789).UTC() nano := now.UnixNano() uuid := "d16ad42a-6fe5-4a3b-8092-69983142238f" @@ -41,6 +45,13 @@ func TestExtractModTimeFromUploadIDDir(t *testing.T) { expectedMod: time.Unix(0, nano), wantZero: false, }, + { + // This valid UUID decodes to bytes containing 'x'. Checking + // for that byte before parsing plaintext loses the timestamp. + name: "Plaintext Also Decodes To Base64 Containing x", + dirName: fmt.Sprintf("4eacd477-e733-4851-b821-6fd42785ff87x%d", nano), + expectedMod: now, + }, { name: "Legacy Format Without Timestamp ()", dirName: uuid, @@ -53,12 +64,27 @@ func TestExtractModTimeFromUploadIDDir(t *testing.T) { expectedMod: time.Unix(0, nano), wantZero: false, }, + { + name: "Encoded Format With Deployment Prefix", + dirName: base64.RawURLEncoding.EncodeToString([]byte(fmt.Sprintf("%s.%sx%d", uuid, uuid, nano))), + expectedMod: now, + }, { name: "Malformed Suffix (xinvalid)", dirName: fmt.Sprintf("%sxnotanumber", uuid), expectedMod: time.Time{}, wantZero: true, }, + { + name: "Timestamp Overflow", + dirName: uuid + "x9223372036854775808", + wantZero: true, + }, + { + name: "Malformed Plaintext UUID", + dirName: fmt.Sprintf("not-a-uuidx%d", nano), + wantZero: true, + }, { name: "Empty Directory Name", dirName: "", @@ -82,3 +108,70 @@ func TestExtractModTimeFromUploadIDDir(t *testing.T) { }) } } + +type multipartCleanupDisk struct { + StorageAPI + root string + modTime time.Time + readErr error + reads int +} + +func (d *multipartCleanupDisk) Endpoint() Endpoint { + return Endpoint{URL: &url.URL{Path: d.root}} +} + +func (d *multipartCleanupDisk) ReadVersion(context.Context, string, string, string, string, ReadOptions) (FileInfo, error) { + d.reads++ + return FileInfo{ModTime: d.modTime}, d.readErr +} + +func (d *multipartCleanupDisk) StatVol(context.Context, string) (VolInfo, error) { + return VolInfo{Created: time.Now()}, nil +} + +func TestCleanupStaleUploadsOnDisk(t *testing.T) { + const uuid = "d16ad42a-6fe5-4a3b-8092-69983142238f" + expiry := globalAPIConfig.getStaleUploadsExpiry() + for _, tc := range []struct { + name, prefix string + age time.Duration + legacy bool + readErr error + wantReads int + wantDeleted bool + }{ + {name: "recent", prefix: uuid, age: expiry / 2, readErr: errFileNotFound}, + {name: "plaintext_base64_collision", prefix: "4eacd477-e733-4851-b821-6fd42785ff87", age: expiry / 2, readErr: errFileNotFound}, + {name: "expired", prefix: uuid, age: 2 * expiry, readErr: errFileNotFound, wantDeleted: true}, + {name: "legacy_recent", prefix: uuid, age: expiry / 2, legacy: true, wantReads: 1}, + {name: "legacy_expired", prefix: uuid, age: 2 * expiry, legacy: true, wantReads: 1, wantDeleted: true}, + {name: "legacy_metadata_missing", prefix: uuid, age: 2 * expiry, legacy: true, readErr: errFileNotFound, wantReads: 1}, + {name: "malformed_metadata_missing", prefix: "not-a-uuid", age: 2 * expiry, readErr: errFileNotFound, wantReads: 1}, + } { + t.Run(tc.name, func(t *testing.T) { + modTime := time.Now().Add(-tc.age) + id := tc.prefix + if !tc.legacy { + id = fmt.Sprintf("%sx%d", tc.prefix, modTime.UnixNano()) + } + disk := &multipartCleanupDisk{root: t.TempDir(), modTime: modTime, readErr: tc.readErr} + uploadDir := filepath.Join(disk.root, minioMetaMultipartBucket, "sha", id) + if err := os.MkdirAll(uploadDir, 0o755); err != nil { + t.Fatal(err) + } + erasureObjects{}.cleanupStaleUploadsOnDisk(t.Context(), disk) + if disk.reads != tc.wantReads { + t.Errorf("ReadVersion calls = %d, want %d", disk.reads, tc.wantReads) + } + _, err := os.Stat(uploadDir) + if tc.wantDeleted { + if !os.IsNotExist(err) { + t.Errorf("expired upload must be removed: %v", err) + } + } else if err != nil { + t.Errorf("upload must be preserved: %v", err) + } + }) + } +} diff --git a/cmd/erasure-multipart.go b/cmd/erasure-multipart.go index db047b59e..5e57cbd17 100644 --- a/cmd/erasure-multipart.go +++ b/cmd/erasure-multipart.go @@ -176,16 +176,17 @@ func (er erasureObjects) deleteAll(ctx context.Context, bucket, prefix string) { // extractModTimeFromUploadIDDir extracts creation time from the upload ID directory on disk. // On-disk directory names are of the form x. -// For compatibility with legacy or encoded representations, if the directory is Base64 encoded, -// it decodes the name before parsing. +// Parse the native format first: plaintext UUIDs can also be valid Base64. +// Preserve the previous encoded-format parser for compatibility. func extractModTimeFromUploadIDDir(uploadIDDir string) time.Time { - target := uploadIDDir - if b64, err := base64.RawURLEncoding.DecodeString(uploadIDDir); err == nil && strings.Contains(string(b64), "x") { - target = string(b64) + if modTime, ok := multipartUploadTime(uploadIDDir); ok { + return modTime } - if split := strings.Split(target, "x"); len(split) == 2 { - if t, err := strconv.ParseInt(split[1], 10, 64); err == nil { - return time.Unix(0, t) + if b64, err := base64.RawURLEncoding.DecodeString(uploadIDDir); err == nil { + if split := strings.Split(string(b64), "x"); len(split) == 2 { + if t, err := strconv.ParseInt(split[1], 10, 64); err == nil { + return time.Unix(0, t) + } } } return time.Time{}