diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go index 884fa9c83..fa35d9058 100644 --- a/cmd/auth-handler.go +++ b/cmd/auth-handler.go @@ -632,18 +632,11 @@ func isReqAuthenticated(ctx context.Context, r *http.Request, region string, sty return ErrInvalidDigest } - // Extract either 'X-Amz-Content-Sha256' header or 'X-Amz-Content-Sha256' query parameter (if V4 presigned) - // Do not verify 'X-Amz-Content-Sha256' if skipSHA256. + // Honor the selected header/query checksum, including the header fallback + // for a presigned request. STS separately hashes its body for its signature. var contentSHA256 []byte - if skipSHA256 := skipContentSha256Cksum(r); !skipSHA256 && isRequestPresignedSignatureV4(r) { - if sha256Sum, ok := r.Form[xhttp.AmzContentSha256]; ok && len(sha256Sum) > 0 { - contentSHA256, err = hex.DecodeString(sha256Sum[0]) - if err != nil { - return ErrContentSHA256Mismatch - } - } - } else if _, ok := r.Header[xhttp.AmzContentSha256]; !skipSHA256 && ok { - contentSHA256, err = hex.DecodeString(r.Header.Get(xhttp.AmzContentSha256)) + if !skipContentSha256Cksum(r) { + contentSHA256, err = hex.DecodeString(getContentSha256Cksum(r, serviceS3)) if err != nil || len(contentSHA256) == 0 { return ErrContentSHA256Mismatch } diff --git a/cmd/bucket-object-lock.go b/cmd/bucket-object-lock.go index e5828e2dd..36211c45d 100644 --- a/cmd/bucket-object-lock.go +++ b/cmd/bucket-object-lock.go @@ -334,11 +334,10 @@ func checkPutObjectLockAllowed(ctx context.Context, rq *http.Request, bucket, ob return mode, retainDate, legalHold, ErrObjectLocked } - if !legalHoldRequested && retentionCfg.LockEnabled { - // inherit retention from bucket configuration - return retentionCfg.Mode, objectlock.RetentionDate{Time: t.Add(retentionCfg.Validity)}, legalHold, ErrNone - } - return "", objectlock.RetentionDate{}, legalHold, ErrNone + // Inherit retention from the bucket configuration. A legal-hold header + // on the same request, ON or OFF, is independent of retention and must + // not suppress the default (#165). + return retentionCfg.Mode, objectlock.RetentionDate{Time: t.Add(retentionCfg.Validity)}, legalHold, ErrNone } return mode, retainDate, legalHold, ErrNone } diff --git a/cmd/bucket-policy.go b/cmd/bucket-policy.go index 9657abf98..567ab0a1c 100644 --- a/cmd/bucket-policy.go +++ b/cmd/bucket-policy.go @@ -255,13 +255,32 @@ func getConditionValuesWithTags(r *http.Request, lc string, cred auth.Credential } cloneHeader := r.Header.Clone() - signatureAge := cloneHeader.Get(xhttp.AmzSignatureAge) - cloneHeader.Del(xhttp.AmzSignatureAge) - // The presigned V4 verifier overwrites this internal scratch header after - // validating the signature. Ignore a value supplied on every other request - // type, where it would otherwise synthesize s3:signatureAge. - if authType == authTypePresigned && signatureAge != "" { - args["signatureAge"] = []string{signatureAge} + + // s3:signatureAge is derived from the presigned X-Amz-Date rather than from + // anything the verifier writes back: PutObject and UploadPart authorize + // before they verify the signature, so a post-verification value is not yet + // available on the first evaluation. The date is bound by the signature + // (doesPresignedSignatureMatch rebuilds and compares it), so a forged date + // only changes the authorization outcome of a request that then fails + // verification. A date that does not parse leaves the key absent; the + // verifier rejects the request as ErrMalformedPresignedDate. + if authType == authTypePresigned { + if signedDate, err := time.Parse(iso8601Format, r.Form.Get(xhttp.AmzDate)); err == nil { + args["signatureAge"] = []string{strconv.FormatInt(currTime.Sub(signedDate).Milliseconds(), 10)} + } + } + + // s3:x-amz-content-sha256 must name the payload hash the request is actually + // verified and enforced against, and only one such value. Presence of the + // header controls whether the key exists at all (AWS documents that the + // query-string form does not populate it), but the value comes from the same + // selection getContentSha256Cksum makes for verification: the presigned query + // value takes precedence over the header, and a repeated header contributes + // only its first value. Exposing every raw header value instead let a + // request satisfy a policy with a value the verifier never checked. + if _, ok := cloneHeader[xhttp.AmzContentSha256]; ok { + args[xhttp.AmzContentSha256] = []string{getContentSha256Cksum(r, serviceS3)} + cloneHeader.Del(xhttp.AmzContentSha256) } userTags := cloneHeader.Get(xhttp.AmzObjectTagging) diff --git a/cmd/bucket-policy_test.go b/cmd/bucket-policy_test.go index 7bd5bb824..42dbb1d8a 100644 --- a/cmd/bucket-policy_test.go +++ b/cmd/bucket-policy_test.go @@ -23,8 +23,10 @@ import ( "net/url" "os" "slices" + "strconv" "strings" "testing" + "time" "github.com/minio/minio/internal/auth" "github.com/minio/minio/internal/handlers" @@ -579,8 +581,20 @@ func TestGetConditionValuesRejectsAbsentInternalKeys(t *testing.T) { } } +// s3:signatureAge is derived from the presigned X-Amz-Date, which the signature +// binds. A client header under the former scratch name must never supply it on +// any auth type, and a presign whose date is missing or malformed leaves the +// key absent (the verifier then rejects the request). func TestGetConditionValuesOnlyAcceptsPresignedSignatureAge(t *testing.T) { const signatureAgeHeader = "x-amz-signature-age" + signedDate := UTCNow().Add(-90 * time.Second) + presignQuery := func(date string) string { + q := url.Values{xhttp.AmzCredential: {"access/20260803/us-east-1/s3/aws4_request"}} + if date != "" { + q.Set(xhttp.AmzDate, date) + } + return "http://minio.local/bkt/obj?" + q.Encode() + } for _, tc := range []struct { name string @@ -602,19 +616,33 @@ func TestGetConditionValuesOnlyAcceptsPresignedSignatureAge(t *testing.T) { }, }, { - name: "presigned verifier value", - target: "http://minio.local/bkt/obj?" + url.Values{ - xhttp.AmzCredential: {"access/20260803/us-east-1/s3/aws4_request"}, - }.Encode(), + name: "presigned client header without date", + target: presignQuery(""), + headers: map[string]string{signatureAgeHeader: "250"}, + }, + { + name: "presigned malformed date", + target: presignQuery("yesterday"), + }, + { + name: "presigned signed date", + target: presignQuery(signedDate.Format(iso8601Format)), headers: map[string]string{signatureAgeHeader: "250"}, want: true, }, } { t.Run(tc.name, func(t *testing.T) { got := condValuesForRequest(t, tc.target, tc.headers) - _, ok := got["signatureAge"] + v, ok := got["signatureAge"] if ok != tc.want { - t.Fatalf("signatureAge presence: expected %v, got %v", tc.want, got["signatureAge"]) + t.Fatalf("signatureAge presence: expected %v, got %v", tc.want, v) + } + if !tc.want { + return + } + age, err := strconv.ParseInt(strings.Join(v, ""), 10, 64) + if err != nil || age < (90*time.Second).Milliseconds() || age > (2*time.Minute).Milliseconds() { + t.Fatalf("signatureAge = %v, want about 90s derived from X-Amz-Date rather than the client header", v) } }) } diff --git a/cmd/object-copy-federation-legalhold_test.go b/cmd/object-copy-federation-legalhold_test.go new file mode 100644 index 000000000..941719699 --- /dev/null +++ b/cmd/object-copy-federation-legalhold_test.go @@ -0,0 +1,241 @@ +// Copyright (c) 2015-2026 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 ( + "bytes" + "net/http" + "strings" + "testing" + "time" + + "github.com/minio/minio/internal/auth" + objectlock "github.com/minio/minio/internal/bucket/object/lock" + xhttp "github.com/minio/minio/internal/http" + "github.com/minio/minio/internal/kms" +) + +// enableBucketObjectLock puts a lock-enabled configuration on an existing +// bucket so checkPutObjectLockAllowed accepts a legal-hold header instead of +// rejecting the request with ErrInvalidBucketObjectLockConfiguration. +func enableBucketObjectLock(t *testing.T, bucket string) { + t.Helper() + meta, err := globalBucketMetadataSys.Get(bucket) + if err != nil { + t.Fatalf("unable to read bucket metadata for %s: %v", bucket, err) + } + updated := meta + updated.ObjectLockConfigXML = enabledBucketObjectLockConfig + updated.VersioningConfigXML = enabledBucketVersioningConfig + // The XML alone is not enough: BucketMetadata keeps a parsed copy that the + // lookups actually read, and it is only populated by parseAllConfigs. + if err := updated.parseAllConfigs(t.Context(), newObjectLayerFn()); err != nil { + t.Fatalf("unable to parse bucket metadata for %s: %v", bucket, err) + } + globalBucketMetadataSys.Set(bucket, updated) +} + +// Exercise #165 and #166 together, including hold OFF, default retention and +// explicit subsecond retention. Ordinary copies must not inherit source locks. +func TestAPIFederatedCopyObjectLockParity(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + endpoints: []string{"CopyObject", "PutObject", "HeadObject", "GetObject"}, + objAPITest: func(obj ObjectLayer, instanceType, bucket string, router http.Handler, cred auth.Credentials, t *testing.T) { + testKMS, err := kms.NewBuiltin(federationTestKMSKeyID, bytes.Repeat([]byte{0x58}, 32)) + if err != nil { + t.Fatal(err) + } + previousKMS := GlobalKMS + GlobalKMS = testKMS + defer func() { GlobalKMS = previousKMS }() + remoteBucket, capture, cleanup := setupCopyObjectFederation(t, obj, router, instanceType, bucket) + defer cleanup() + enableBucketObjectLock(t, bucket) + until := UTCNow().Add(7 * 24 * time.Hour).Truncate(time.Second).Add(789 * time.Millisecond).Format(time.RFC3339Nano) + for _, sourceType := range []string{"plain", "s3"} { + source := sourceType + "-held-source" + headers := federationSSEHeaders(sourceType, 0, false) + headers[xhttp.AmzObjectLockLegalHold] = "ON" + putCopyChecksumSource(t, router, cred, bucket, source, []byte("held source"), headers) + for _, tc := range []struct { + name, hold, defaultMode, explicitMode string + replace bool + }{ + {name: "on", hold: "ON"}, + {name: "off", hold: "OFF"}, + {name: "no source inheritance"}, + {name: "on with default", hold: "ON", defaultMode: "GOVERNANCE"}, + {name: "off with default", hold: "OFF", defaultMode: "COMPLIANCE"}, + {name: "explicit retention", explicitMode: "GOVERNANCE"}, + {name: "hold and explicit retention", hold: "ON", explicitMode: "COMPLIANCE"}, + {name: "replace metadata", hold: "ON", explicitMode: "GOVERNANCE", replace: true}, + } { + t.Run(instanceType+"/"+sourceType+"/"+tc.name, func(t *testing.T) { + setTestBucketDefaultRetention(t, bucket, tc.defaultMode) + setTestBucketDefaultRetention(t, remoteBucket, tc.defaultMode) + headers := map[string]string{} + if tc.hold != "" { + headers[xhttp.AmzObjectLockLegalHold] = tc.hold + } + if tc.explicitMode != "" { + headers[xhttp.AmzObjectLockMode] = tc.explicitMode + headers[xhttp.AmzObjectLockRetainUntilDate] = until + } + if tc.replace { + headers[xhttp.AmzMetadataDirective] = "REPLACE" + headers["X-Amz-Meta-Origin"] = "replacement" + } + capture.mu.Lock() + capture.headers = nil + capture.mu.Unlock() + for _, destinationBucket := range []string{bucket, remoteBucket} { + destination := sourceType + "-copy-" + strings.ReplaceAll(tc.name, " ", "-") + rec := federatedCopyRequest(t, router, cred, bucket, source, destinationBucket, destination, headers) + if rec.Code != http.StatusOK { + t.Fatalf("copy to %s: %d %s", destinationBucket, rec.Code, rec.Body.String()) + } + info, err := obj.GetObjectInfo(t.Context(), destinationBucket, destination, ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + if got := objectlock.GetObjectLegalHoldMeta(info.UserDefined).Status; string(got) != tc.hold { + t.Errorf("stored hold = %q, want %q", got, tc.hold) + } + retention := objectlock.GetObjectRetentionMeta(info.UserDefined) + mode := tc.explicitMode + if mode == "" { + mode = tc.defaultMode + } + if string(retention.Mode) != mode { + t.Errorf("stored retention = %q, want %q", retention.Mode, mode) + } + if tc.explicitMode != "" && retention.RetainUntilDate.Format(time.RFC3339Nano) != until { + t.Errorf("retention date lost precision: %s, want %s", retention.RetainUntilDate, until) + } + for key := range info.UserDefined { + if stringsHasPrefixFold(key, "X-Amz-Meta-X-Amz-Object-Lock-") { + t.Errorf("lock state became user metadata: %s", key) + } + } + if tc.replace && info.UserDefined["X-Amz-Meta-Origin"] != "replacement" { + t.Errorf("replacement metadata was lost: %v", info.UserDefined) + } + } + typed, asMetadata := capture.legalHoldHeaders() + if len(asMetadata) != 0 || (tc.hold != "" && strings.Join(typed, "") != tc.hold) || (tc.hold == "" && len(typed) != 0) { + t.Errorf("forwarded hold headers = %v, metadata = %v; want %q", typed, asMetadata, tc.hold) + } + }) + } + } + }, + }) +} + +// legalHoldHeaders returns the forwarded legal-hold headers the remote saw, +// separated into the real Object Lock header and the user-metadata spelling +// minio-go produces for an unrecognized UserMetadata key. +func (c *federationRemoteCapture) legalHoldHeaders() (typed, asMetadata []string) { + c.mu.Lock() + defer c.mu.Unlock() + metaKey := "X-Amz-Meta-" + xhttp.AmzObjectLockLegalHold + for _, h := range c.headers { + for k, v := range h { + switch { + case strings.EqualFold(k, xhttp.AmzObjectLockLegalHold): + typed = append(typed, strings.Join(v, ",")) + case strings.EqualFold(k, metaKey): + asMetadata = append(asMetadata, strings.Join(v, ",")) + } + } + } + return typed, asMetadata +} + +// TestAPIFederatedCopyObjectLegalHold drives the legacy etcd federation branch +// of CopyObjectHandler with an explicit legal hold on the copy. +// +// Before the fix the resolved hold was forwarded inside +// PutObjectOptions.UserMetadata. minio-go's Header() prefixes every +// UserMetadata key it does not recognize with "x-amz-meta-", and +// x-amz-object-lock-legal-hold is in neither supportedHeaders nor isAmzHeader, +// so the hold reached the remote as X-Amz-Meta-X-Amz-Object-Lock-Legal-Hold. +// The destination stored no hold and the copy still answered 200 -- a silent +// loss of a WORM control (#166). Retention requested on the same copy survived, +// which is what made it easy to miss. +func TestAPIFederatedCopyObjectLegalHold(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPIFederatedCopyObjectLegalHold, + endpoints: []string{"CopyObject", "PutObject", "HeadObject", "GetObject"}, + }) +} + +func testAPIFederatedCopyObjectLegalHold(objectAPI ObjectLayer, instanceType, bucketName string, + apiRouter http.Handler, credentials auth.Credentials, t *testing.T, +) { + data := []byte("federated copy with a legal hold") + srcObject := "federation/legal-hold-source" + putCopyChecksumSource(t, apiRouter, credentials, bucketName, srcObject, data, nil) + + remoteBucket, capture, cleanup := setupCopyObjectFederation(t, objectAPI, apiRouter, instanceType, bucketName) + defer cleanup() + + // Both roles read bucket metadata from the shared backend in this fixture, + // so one configuration covers the proxy's own check and the remote's. + enableBucketObjectLock(t, bucketName) + enableBucketObjectLock(t, remoteBucket) + + dstObject := "federation/legal-hold-destination" + rec := federatedCopyRequest(t, apiRouter, credentials, bucketName, srcObject, remoteBucket, dstObject, + map[string]string{xhttp.AmzObjectLockLegalHold: string(objectlock.LegalHoldOn)}) + if rec.Code != http.StatusOK { + t.Fatalf("%s: federated CopyObject with a legal hold failed: %d %s", + instanceType, rec.Code, rec.Body.String()) + } + + // The wire is the point: the hold must arrive as the Object Lock header, + // never as user metadata. A 200 with the metadata spelling is exactly the + // silent loss this test exists for. + typed, asMetadata := capture.legalHoldHeaders() + if len(asMetadata) != 0 { + t.Fatalf("%s: legal hold forwarded as user metadata %v; the destination stores no hold", + instanceType, asMetadata) + } + if len(typed) == 0 { + t.Fatalf("%s: no %s header reached the remote deployment", instanceType, xhttp.AmzObjectLockLegalHold) + } + for _, got := range typed { + if !strings.EqualFold(got, string(objectlock.LegalHoldOn)) { + t.Fatalf("%s: forwarded legal hold = %q, want %q", instanceType, got, objectlock.LegalHoldOn) + } + } + + // And it must actually be stored on the destination version. + oi, err := objectAPI.GetObjectInfo(t.Context(), remoteBucket, dstObject, ObjectOptions{}) + if err != nil { + t.Fatalf("%s: unable to stat the federated copy destination: %v", instanceType, err) + } + if hold := objectlock.GetObjectLegalHoldMeta(oi.UserDefined); hold.Status != objectlock.LegalHoldOn { + t.Fatalf("%s: destination legal hold = %q, want %q (metadata: %v)", + instanceType, hold.Status, objectlock.LegalHoldOn, oi.UserDefined) + } +} diff --git a/cmd/object-copy-federation-result_test.go b/cmd/object-copy-federation-result_test.go new file mode 100644 index 000000000..20d70f10b --- /dev/null +++ b/cmd/object-copy-federation-result_test.go @@ -0,0 +1,158 @@ +// Copyright (c) 2026 PGSTY +// SPDX-License-Identifier: AGPL-3.0-only + +package cmd + +import ( + "bytes" + "net/http" + "net/url" + "strings" + "testing" + "time" + + "github.com/minio/minio/internal/auth" + sse "github.com/minio/minio/internal/bucket/encryption" + "github.com/minio/minio/internal/event" + xhttp "github.com/minio/minio/internal/http" + "github.com/minio/minio/internal/kms" + "github.com/minio/minio/internal/pubsub" +) + +func TestAPIFederatedCopyObjectVersionAndEvent(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + endpoints: []string{"CopyObject", "PutObject", "GetObject", "HeadObject"}, + objAPITest: func(obj ObjectLayer, instanceType, bucket string, router http.Handler, cred auth.Credentials, t *testing.T) { + testKMS, err := kms.NewBuiltin(federationTestKMSKeyID, bytes.Repeat([]byte{0x58}, 32)) + if err != nil { + t.Fatal(err) + } + previousKMS := GlobalKMS + GlobalKMS = testKMS + defer func() { GlobalKMS = previousKMS }() + restore := setCopyChecksumCompression(true) + defer restore() + remoteBucket, _, cleanup := setupCopyObjectFederation(t, obj, router, instanceType, bucket) + defer cleanup() + enableBucketObjectLock(t, remoteBucket) + events := make(chan event.Event, 8) + done := make(chan struct{}) + defer close(done) + if err := globalHTTPListen.Subscribe(pubsub.MaskFromMaskable(event.ObjectCreatedCopy), events, done, nil); err != nil { + t.Fatal(err) + } + for _, kind := range []string{"plain", "compressed", "encrypted"} { + t.Run(instanceType+"/"+kind, func(t *testing.T) { + data := []byte("logical object size") + source := kind + "-source.bin" + var headers map[string]string + if kind == "compressed" { + data = bytes.Repeat(data, 8192) + source = kind + "-source.txt" + } + if kind == "encrypted" { + headers = federationSSEHeaders("s3", 0, false) + } + putCopyChecksumSource(t, router, cred, bucket, source, data, headers) + destination := "result/" + kind + " with space.bin" + rec := federatedCopyRequest(t, router, cred, bucket, source, remoteBucket, destination, nil) + if rec.Code != http.StatusOK { + t.Fatalf("copy: %d %s", rec.Code, rec.Body.String()) + } + versionID := strings.Join(rec.Header()[xhttp.AmzVersionID], "") + if versionID == "" { + t.Error("copy response omitted destination version ID") + } else { + info, err := obj.GetObjectInfo(t.Context(), remoteBucket, destination, ObjectOptions{VersionID: versionID}) + if err != nil || info.VersionID != versionID { + t.Fatalf("response does not name the written version: %v, %q", err, info.VersionID) + } + } + select { + case evt := <-events: + key, err := url.QueryUnescape(evt.S3.Object.Key) + if err != nil || key != destination || evt.S3.Bucket.Name != remoteBucket || evt.S3.Object.Size != int64(len(data)) || evt.S3.Object.VersionID == "" || evt.S3.Object.VersionID != versionID { + t.Errorf("copy event does not describe the written object: %+v", evt.S3) + } + case <-time.After(5 * time.Second): + t.Fatal("copy event was not emitted") + } + }) + } + }, + }) +} + +func TestAPIFederatedCopyObjectDestinationSSEDefaults(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + endpoints: []string{"CopyObject", "PutObject", "GetObject", "HeadObject"}, + objAPITest: func(obj ObjectLayer, instanceType, bucket string, router http.Handler, cred auth.Credentials, t *testing.T) { + data := []byte("destination chooses default encryption") + putCopyChecksumSource(t, router, cred, bucket, "source", data, nil) + testKMS, err := kms.NewBuiltin(federationTestKMSKeyID, bytes.Repeat([]byte{0x58}, 32)) + if err != nil { + t.Fatal(err) + } + previousKMS, previousAuto := GlobalKMS, globalAutoEncryption + GlobalKMS, globalAutoEncryption = testKMS, true + defer func() { GlobalKMS, globalAutoEncryption = previousKMS, previousAuto }() + remoteBucket, capture, cleanup := setupCopyObjectFederationRemote(t, obj, router, instanceType, bucket, true) + defer cleanup() + destinationConfig, err := sse.ParseBucketSSEConfig(strings.NewReader(`aws:kmsdestination-bucket-key`)) + if err != nil { + t.Fatal(err) + } + for _, kind := range []string{"plain", "s3", "kms", "kms-default", "c"} { + t.Run(instanceType+"/"+kind, func(t *testing.T) { + var headers map[string]string + if kind == "kms-default" { + headers = map[string]string{xhttp.AmzServerSideEncryption: xhttp.AmzEncryptionKMS} + } else { + headers = federationSSEHeaders(kind, 0x22, false) + } + rec := federatedCopyRequest(t, router, cred, bucket, "source", remoteBucket, kind, headers) + if rec.Code != http.StatusOK { + t.Fatalf("copy: %d %s", rec.Code, rec.Body.String()) + } + capture.mu.Lock() + forwarded := capture.headers[len(capture.headers)-1].Clone() + capture.mu.Unlock() + if kind == "plain" { + if forwarded.Get(xhttp.AmzServerSideEncryption) != "" { + t.Errorf("proxy injected SSE %q into a request with no client SSE", forwarded.Get(xhttp.AmzServerSideEncryption)) + } + // With nothing injected, the KMS encryption the destination + // stores can only be the remote applying its own defaults. + info, err := obj.GetObjectInfo(t.Context(), remoteBucket, kind, ObjectOptions{}) + if err != nil || federationStoredSSE(info.UserDefined) != "kms" { + t.Errorf("remote destination did not apply its own auto-encryption: %v, %v", err, info.UserDefined) + } + } + // Replay the actual inbound headers against an independent bucket + // configuration. No handler flips shared globals while serving. + destinationConfig.Apply(forwarded, sse.ApplyOptions{}) + wantKey := headers[xhttp.AmzServerSideEncryptionKmsID] + if kind == "plain" { + wantKey = "destination-bucket-key" + } + if got := forwarded.Get(xhttp.AmzServerSideEncryptionKmsID); got != wantKey { + t.Errorf("destination selected key %q, want %q", got, wantKey) + } + }) + } + // A local destination still inherits the server's auto-encryption. + rec := federatedCopyRequest(t, router, cred, bucket, "source", bucket, "local-copy", nil) + if rec.Code != http.StatusOK { + t.Fatalf("local copy: %d %s", rec.Code, rec.Body.String()) + } + info, err := obj.GetObjectInfo(t.Context(), bucket, "local-copy", ObjectOptions{}) + if err != nil || federationStoredSSE(info.UserDefined) != "kms" { + t.Errorf("local copy lost auto-encryption: %v, %v", err, info.UserDefined) + } + }, + }) +} diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index c8fbee472..a4dfab00b 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -1404,11 +1404,27 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re } allowReplicationMetadata := replicaTrusted - // Check if bucket encryption is enabled - sseConfig, _ := globalBucketSSEConfigSys.Get(dstBucket) - sseConfig.Apply(r.Header, sse.ApplyOptions{ - AutoEncrypt: globalAutoEncryption, - }) + // Federation only: the destination bucket lives on another deployment and + // the copy is forwarded to it as a PutObject. That remote write owns the + // destination's storage transformations, so this handler hands it the + // logical (decompressed, decrypted) bytes at their logical size and lets + // the remote compress and encrypt once. Encrypting here as well would + // forward ciphertext under the destination's own SSE option, which either + // fails the length check or, for SSE to SSE, has the remote encrypt the + // ciphertext a second time and store an unreadable object (#158). + remoteCallRequired := isRemoteCopyRequired(ctx, srcBucket, dstBucket, objectAPI) + + // Apply the destination bucket's default encryption only when this + // deployment writes the destination. For a remote destination the proxy + // has no authority over that bucket's defaults: applying its own here + // would forward an explicit SSE header that the remote then honors in + // place of the destination's configuration (#167). + if !remoteCallRequired { + sseConfig, _ := globalBucketSSEConfigSys.Get(dstBucket) + sseConfig.Apply(r.Header, sse.ApplyOptions{ + AutoEncrypt: globalAutoEncryption, + }) + } var srcOpts, dstOpts ObjectOptions srcOpts, err = copySrcOpts(ctx, r, srcBucket, srcObject) if err != nil { @@ -1518,16 +1534,6 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re } } - // Federation only: the destination bucket lives on another deployment and - // the copy is forwarded to it as a PutObject. That remote write owns the - // destination's storage transformations, so this handler hands it the - // logical (decompressed, decrypted) bytes at their logical size and lets - // the remote compress and encrypt once. Encrypting here as well would - // forward ciphertext under the destination's own SSE option, which either - // fails the length check or, for SSE to SSE, has the remote encrypt the - // ciphertext a second time and store an unreadable object (#158). - remoteCallRequired := isRemoteCopyRequired(ctx, srcBucket, dstBucket, objectAPI) - var compressMetadata map[string]string // No need to compress for remote etcd calls // Pass the decompressed stream to such calls. @@ -1945,10 +1951,23 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re delete(srcInfo.UserDefined, k) } } + // Forward a clone: request-only keys are removed or added below and + // srcInfo.UserDefined stays the resolved record for the response. + forwardedMeta := cloneMSS(srcInfo.UserDefined) + // minio-go prefixes any UserMetadata key it does not recognize with + // x-amz-meta-, and it recognizes the retention headers but not + // x-amz-object-lock-legal-hold, so a hold left in the map reaches the + // remote as user metadata and is silently dropped (#166). Carry it on + // the typed option instead. Retention stays in the map: the typed + // RetainUntilDate would truncate the date to whole seconds. + legalHoldKey := strings.ToLower(xhttp.AmzObjectLockLegalHold) + forwardedLegalHold := forwardedMeta[legalHoldKey] + delete(forwardedMeta, legalHoldKey) opts := miniogo.PutObjectOptions{ - UserMetadata: srcInfo.UserDefined, + UserMetadata: forwardedMeta, ServerSideEncryption: dstOpts.ServerSideEncryption, UserTags: tag.ToMap(), + LegalHold: miniogo.LegalHoldStatus(forwardedLegalHold), } // The destination must carry the same checksum the local path would // produce; the federated path has the remote compute, validate, persist @@ -1996,11 +2015,18 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re writeErrorResponse(ctx, w, toAPIError(ctx, rerr), r.URL) return } - objInfo.UserDefined = cloneMSS(opts.UserMetadata) - // A forwarded checksum header is a request detail, not object metadata. - if checksumHeaderValue != "" { - delete(objInfo.UserDefined, wantChecksumType.Key()) - } + // Keep the resolved legal hold in response and event metadata; the + // request-only checksum header exists only in the forwarding map. + objInfo.UserDefined = cloneMSS(srcInfo.UserDefined) + // The response headers and the ObjectCreated:Copy event describe the + // object this handler wrote, so name it: without these the response + // carries no x-amz-version-id and the event has an empty key and zero + // size (#170). Size is the logical size the remote was handed, which is + // what it reports back. + objInfo.Bucket = dstBucket + objInfo.Name = dstObject + objInfo.Size = actualSize + objInfo.VersionID = remoteObjInfo.VersionID objInfo.ETag = remoteObjInfo.ETag objInfo.ModTime = remoteObjInfo.LastModified // Bind the checksum the remote computed for this exact write. A single diff --git a/cmd/object-lock-default-retention_test.go b/cmd/object-lock-default-retention_test.go new file mode 100644 index 000000000..325657fc6 --- /dev/null +++ b/cmd/object-lock-default-retention_test.go @@ -0,0 +1,140 @@ +// Copyright (c) 2026 PGSTY +// SPDX-License-Identifier: AGPL-3.0-only + +package cmd + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/minio/minio/internal/auth" + objectlock "github.com/minio/minio/internal/bucket/object/lock" + xhttp "github.com/minio/minio/internal/http" +) + +func setTestBucketDefaultRetention(t *testing.T, bucket, mode string) { + t.Helper() + enableBucketObjectLock(t, bucket) + if mode == "" { + return + } + meta, err := globalBucketMetadataSys.Get(bucket) + if err != nil { + t.Fatal(err) + } + meta.ObjectLockConfigXML = fmt.Appendf(nil, `Enabled%s2`, mode) + if err := meta.parseAllConfigs(t.Context(), newObjectLayerFn()); err != nil { + t.Fatal(err) + } + globalBucketMetadataSys.Set(bucket, meta) +} + +func TestAPIObjectLockDefaultRetentionWithLegalHold(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + endpoints: []string{"NewMultipart", "PutObjectPart", "CompleteMultipart", "CopyObject", "PutObject", "DeleteObject"}, + objAPITest: func(obj ObjectLayer, instanceType, bucket string, router http.Handler, cred auth.Credentials, t *testing.T) { + data := []byte("default retention and legal hold are independent") + putCopyChecksumSource(t, router, cred, bucket, "source", data, nil) + for _, mode := range []string{"GOVERNANCE", "COMPLIANCE", ""} { + setTestBucketDefaultRetention(t, bucket, mode) + for _, hold := range []string{"ON", "OFF"} { + for _, operation := range []string{"put", "copy", "multipart"} { + t.Run(instanceType+"/"+mode+"/"+hold+"/"+operation, func(t *testing.T) { + object := mode + "-" + hold + "-" + operation + headers := map[string]string{xhttp.AmzObjectLockLegalHold: hold} + // The stored retention date carries millisecond precision. + before := UTCNow().Truncate(time.Millisecond) + switch operation { + case "put": + putCopyChecksumSource(t, router, cred, bucket, object, data, headers) + case "copy": + rec := federatedCopyRequest(t, router, cred, bucket, "source", bucket, object, headers) + if rec.Code != http.StatusOK { + t.Fatalf("copy: %d %s", rec.Code, rec.Body.String()) + } + case "multipart": + putFederationMultipartSource(t, router, cred, bucket, object, [][]byte{data}, headers) + } + info, err := obj.GetObjectInfo(t.Context(), bucket, object, ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + ret := objectlock.GetObjectRetentionMeta(info.UserDefined) + if string(ret.Mode) != mode { + t.Errorf("stored retention mode = %q, want %q", ret.Mode, mode) + } + if mode != "" && (ret.RetainUntilDate.Before(before.Add(48*time.Hour)) || ret.RetainUntilDate.After(UTCNow().Add(48*time.Hour))) { + t.Errorf("default retention date = %s, want write time + 2 days", ret.RetainUntilDate) + } + if got := objectlock.GetObjectLegalHoldMeta(info.UserDefined).Status; string(got) != hold { + t.Errorf("stored legal hold = %q, want %q", got, hold) + } + if mode == "COMPLIANCE" && hold == "OFF" { + req, err := newTestSignedRequestV4(http.MethodDelete, getPutObjectURL("", bucket, object)+"?versionId="+info.VersionID, 0, nil, cred.AccessKey, cred.SecretKey, nil) + if err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + if rec.Code != http.StatusBadRequest || !strings.Contains(rec.Body.String(), "InvalidRequest") { + t.Errorf("version DELETE must enforce retention: %d %s", rec.Code, rec.Body.String()) + } + } + }) + } + } + } + }, + }) +} + +func TestObjectLockDefaultRetentionBoundaries(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: func(obj ObjectLayer, instanceType, bucket string, _ http.Handler, _ auth.Credentials, t *testing.T) { + setTestBucketDefaultRetention(t, bucket, "COMPLIANCE") + for _, tc := range []struct { + name string + replica, marker, explicit bool + retentionErr, holdErr APIErrorCode + wantMode objectlock.RetMode + wantErr APIErrorCode + }{ + {name: "trusted replica", replica: true}, + {name: "marker-only ordinary write", marker: true, wantMode: objectlock.RetCompliance}, + {name: "explicit retention", explicit: true, wantMode: objectlock.RetGovernance}, + {name: "retention permission denied", retentionErr: ErrAccessDenied, wantErr: ErrAccessDenied}, + {name: "legal hold permission denied", holdErr: ErrAccessDenied, wantErr: ErrAccessDenied}, + } { + t.Run(instanceType+"/"+tc.name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodPut, "http://minio.local/"+bucket+"/object", nil) + r.Header.Set(xhttp.AmzObjectLockLegalHold, "OFF") + if tc.marker { + r.Header.Set(xhttp.MinIOSourceReplicationRequest, "true") + } + until := UTCNow().Add(24 * time.Hour).Truncate(time.Second).Add(789 * time.Millisecond) + if tc.explicit { + r.Header.Set(xhttp.AmzObjectLockMode, "GOVERNANCE") + r.Header.Set(xhttp.AmzObjectLockRetainUntilDate, until.Format(time.RFC3339Nano)) + } + mode, date, _, code := checkPutObjectLockAllowed(t.Context(), r, bucket, "object", obj.GetObjectInfo, tc.retentionErr, tc.holdErr, tc.replica) + if code != tc.wantErr || mode != tc.wantMode { + t.Fatalf("got mode %s error %s, want mode %s error %s", mode, niceError(code), tc.wantMode, niceError(tc.wantErr)) + } + if tc.explicit && !date.Equal(until) { + t.Errorf("explicit retention lost precision: %s != %s", date, until) + } + if tc.replica && !date.IsZero() { + t.Errorf("replica acquired a destination default: %s", date) + } + }) + } + }, + }) +} diff --git a/cmd/presigned-payload_test.go b/cmd/presigned-payload_test.go new file mode 100644 index 000000000..ef39a0a84 --- /dev/null +++ b/cmd/presigned-payload_test.go @@ -0,0 +1,114 @@ +// Copyright (c) 2026 PGSTY +// SPDX-License-Identifier: AGPL-3.0-only + +package cmd + +import ( + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/minio/minio/internal/auth" + xhttp "github.com/minio/minio/internal/http" +) + +func TestPresignedHeaderPayloadHashIsEnforced(t *testing.T) { + setupSignatureBoundaryTest(t) + digest := getSHA256Hash([]byte("expected")) + for _, tc := range []struct { + name, query, header string + unsigned bool + }{ + {name: "header only", header: digest}, + {name: "query only", query: digest}, + {name: "query wins", query: digest, header: getSHA256Hash([]byte("different"))}, + {name: "unsigned query wins", query: unsignedPayload, header: digest, unsigned: true}, + {name: "unsigned header", header: unsignedPayload, unsigned: true}, + {name: "absent", unsigned: true}, + } { + for _, body := range []string{"expected", "modified"} { + t.Run(tc.name+"/"+body, func(t *testing.T) { + r := httptest.NewRequest(http.MethodPut, "http://minio.local/bucket/object", strings.NewReader(body)) + if tc.header != "" { + r.Header.Set(xhttp.AmzContentSha256, tc.header) + } + if tc.query != "" { + query := r.URL.Query() + query.Set(xhttp.AmzContentSha256, tc.query) + r.URL.RawQuery = query.Encode() + } + presignBoundaryRequest(t, r, UTCNow(), []string{"host"}, globalActiveCred) + if code := isReqAuthenticated(t.Context(), r, globalSite.Region(), serviceS3); code != ErrNone { + t.Fatal(niceError(code)) + } + _, err := io.ReadAll(r.Body) + wantMismatch := body == "modified" && !tc.unsigned + if wantMismatch { + if toAPIErrorCode(t.Context(), err) != ErrContentSHA256Mismatch { + t.Errorf("modified signed payload: got %v, want SHA-256 mismatch", err) + } + } else if err != nil { + t.Errorf("valid payload: %v", err) + } + }) + } + } +} + +func TestAPIPresignedBucketPolicyPayloadHash(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: func(objectAPI ObjectLayer, instanceType, bucket string, apiRouter http.Handler, credentials auth.Credentials, t *testing.T) { + server := httptest.NewServer(apiRouter) + defer server.Close() + client := server.Client() + client.Timeout = 10 * time.Second + expected := fmt.Sprintf(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetObject"],"Resource":["arn:aws:s3:::%s/*"]}]}`, bucket) + modified := strings.Replace(expected, "s3:GetObject", "s3:PutObject", 1) + for _, tc := range []struct { + name, body string + status int + }{ + {"original", expected, http.StatusNoContent}, + {"modified", modified, http.StatusBadRequest}, + } { + t.Run(instanceType+"/"+tc.name, func(t *testing.T) { + r, err := http.NewRequestWithContext(t.Context(), http.MethodPut, server.URL+"/"+bucket+"?policy=", strings.NewReader(tc.body)) + if err != nil { + t.Fatal(err) + } + r.Header.Set(xhttp.AmzContentSha256, getSHA256Hash([]byte(expected))) + presignBoundaryRequest(t, r, UTCNow(), []string{"host"}, credentials) + response, err := client.Do(r) + if err != nil { + t.Fatal(err) + } + defer response.Body.Close() + body, err := io.ReadAll(response.Body) + if err != nil { + t.Fatal(err) + } + if response.StatusCode != tc.status { + t.Fatalf("PutBucketPolicy: got %d %s, want %d", response.StatusCode, body, tc.status) + } + if tc.status == http.StatusBadRequest && !strings.Contains(string(body), "XAmzContentSHA256Mismatch") { + t.Fatalf("expected payload checksum rejection, got %s", body) + } + }) + } + meta, err := globalBucketMetadataSys.Get(bucket) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(meta.PolicyConfigJSON), "s3:PutObject") { + t.Fatal("tampered presigned request replaced the bucket policy") + } + }, + endpoints: []string{"PutBucketPolicy"}, + }) +} diff --git a/cmd/signature-v4-boundary_test.go b/cmd/signature-v4-boundary_test.go new file mode 100644 index 000000000..b513b8d71 --- /dev/null +++ b/cmd/signature-v4-boundary_test.go @@ -0,0 +1,324 @@ +// Copyright (c) 2026 PGSTY +// SPDX-License-Identifier: AGPL-3.0-only + +package cmd + +import ( + "bytes" + "encoding/xml" + "fmt" + "net/http" + "net/http/httptest" + "os" + "slices" + "strconv" + "strings" + "testing" + "time" + + "github.com/minio/madmin-go/v3" + "github.com/minio/minio/internal/auth" + xhttp "github.com/minio/minio/internal/http" + "github.com/pgsty/silo-pkg/v3/policy" +) + +// forgedSignatureAgeHeader is the name of the scratch header the presigned +// verifier once wrote s3:signatureAge through. Production code no longer knows +// it; a client that sends it is sending an ordinary unsigned x-amz-* header. +const forgedSignatureAgeHeader = "X-Amz-Signature-Age" + +// Presign at a chosen time, including exactly the supplied operation headers. +func presignBoundaryRequest(t *testing.T, r *http.Request, date time.Time, signedHeaders []string, cred auth.Credentials) { + t.Helper() + query := r.URL.Query() + query.Del(xhttp.AmzSignature) + query.Set(xhttp.AmzAlgorithm, signV4Algorithm) + query.Set(xhttp.AmzDate, date.Format(iso8601Format)) + query.Set(xhttp.AmzExpires, "3600") + query.Set(xhttp.AmzSignedHeaders, strings.Join(signedHeaders, ";")) + query.Set(xhttp.AmzCredential, cred.AccessKey+"/"+getScope(date, globalSite.Region())) + r.Form = query + headers, code := extractSignedHeaders(signedHeaders, r) + if code != ErrNone { + t.Fatal(niceError(code)) + } + canonical := getCanonicalRequest(headers, getContentSha256Cksum(r, serviceS3), query.Encode(), r.URL.Path, r.Method) + key := getSigningKey(cred.SecretKey, date, globalSite.Region(), serviceS3) + query.Set(xhttp.AmzSignature, getSignature(key, getStringToSign(canonical, date, getScope(date, globalSite.Region())))) + r.URL.RawQuery = query.Encode() + r.Form = query +} + +func setupSignatureBoundaryTest(t *testing.T) { + t.Helper() + obj, fsDir, err := prepareFS(t.Context()) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.RemoveAll(fsDir) }) + if err = newTestConfig(globalMinioDefaultRegion, obj); err != nil { + t.Fatal(err) + } +} + +// Canonicalization comma-joins repeated header fields, so a signature over one +// x-amz-copy-source value containing a comma also covers the same text split +// into two fields, while the copy handlers act on Header.Get alone. A single +// value may contain a literal or percent-encoded comma; a repeated header is +// rejected at the shared SigV4 boundary for both signed and presigned requests. +func TestV4CopySourceMultiplicity(t *testing.T) { + setupSignatureBoundaryTest(t) + for _, presigned := range []bool{false, true} { + for _, source := range []string{"/source/allowed,tail", "/source/allowed%2Ctail"} { + t.Run(fmt.Sprintf("presigned=%v/%s", presigned, source), func(t *testing.T) { + r := httptest.NewRequest(http.MethodPut, "http://minio.local/destination/object", nil) + r.Header.Set(xhttp.AmzContentSha256, emptySHA256) + r.Header.Set(xhttp.AmzCopySource, source) + if presigned { + presignBoundaryRequest(t, r, UTCNow(), []string{"host", "x-amz-copy-source"}, globalActiveCred) + } else { + if err := signRequestV4(r, globalActiveCred.AccessKey, globalActiveCred.SecretKey); err != nil { + t.Fatal(err) + } + r.Form = r.URL.Query() + } + if code := reqSignatureV4Verify(r, globalSite.Region(), serviceS3); code != ErrNone { + t.Fatalf("a single source key containing a comma must remain valid: %s", niceError(code)) + } + if source != "/source/allowed,tail" { + return + } + r.Header[xhttp.AmzCopySource] = []string{"/source/allowed", "tail"} + if code := reqSignatureV4Verify(r, globalSite.Region(), serviceS3); code != ErrInvalidCopySource { + t.Fatalf("split copy source: got %s, want %s; handler would copy %q", + niceError(code), niceError(ErrInvalidCopySource), r.Header.Get(xhttp.AmzCopySource)) + } + }) + } + } +} + +// PutObject and UploadPart authorize before they verify the signature, so +// s3:signatureAge must come from the signed X-Amz-Date on the first policy +// evaluation. A client header under the former scratch name must neither +// supply the value nor survive verification, and verifying the same request +// twice must give the same answer. +func TestGetConditionValuesPresignedAgeFromDate(t *testing.T) { + setupSignatureBoundaryTest(t) + for _, tc := range []struct { + name, header string + age time.Duration + wantVerify APIErrorCode + }{ + {name: "old", age: 10 * time.Minute}, + {name: "old forged 0", age: 10 * time.Minute, header: "0", wantVerify: ErrUnsignedHeaders}, + {name: "old forged 1", age: 10 * time.Minute, header: "1", wantVerify: ErrUnsignedHeaders}, + // A signer slightly ahead of the server stays within globalMaxSkewTime. + {name: "future within skew", age: -2 * time.Minute}, + } { + t.Run(tc.name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodPut, "http://minio.local/bucket/object", nil) + presignBoundaryRequest(t, r, UTCNow().Add(-tc.age), []string{"host"}, globalActiveCred) + if tc.header != "" { + r.Header.Set(forgedSignatureAgeHeader, tc.header) + } + values := getConditionValues(r, "", globalActiveCred) + age, err := strconv.ParseInt(strings.Join(values["signatureAge"], ""), 10, 64) + lo, hi := (tc.age - time.Minute).Milliseconds(), (tc.age + time.Minute).Milliseconds() + if err != nil || age < lo || age > hi { + t.Errorf("pre-verification policy got age %v; want the age of the signed date (about %d ms)", values["signatureAge"], tc.age.Milliseconds()) + } + code := reqSignatureV4Verify(r, globalSite.Region(), serviceS3) + if code != tc.wantVerify { + t.Fatalf("verification: got %s, want %s", niceError(code), niceError(tc.wantVerify)) + } + if again := reqSignatureV4Verify(r, globalSite.Region(), serviceS3); again != code { + t.Fatalf("second verification changed the outcome: %s -> %s", niceError(code), niceError(again)) + } + }) + } +} + +// The s3:x-amz-content-sha256 policy value must be the single payload hash the +// request is verified and enforced against. Header presence decides whether the +// key exists at all; the value is the one getContentSha256Cksum selects, so a +// presigned query value wins over the header and a repeated header contributes +// only its first value. None of these requests is rejected at the protocol +// level; the policy simply sees what verification bound. +func TestGetConditionValuesPayloadHashMatchesVerifiedValue(t *testing.T) { + setupSignatureBoundaryTest(t) + hashA, hashB := getSHA256Hash([]byte("a")), getSHA256Hash([]byte("b")) + for _, tc := range []struct { + name string + presigned bool + query string + header []string + want []string + }{ + {name: "signed header", header: []string{hashA}, want: []string{hashA}}, + {name: "signed absent"}, + {name: "signed present empty", header: []string{""}, want: []string{""}}, + {name: "signed duplicate header", header: []string{hashA, hashB}, want: []string{hashA}}, + {name: "signed streaming with second value", header: []string{streamingContentSHA256, hashA}, want: []string{streamingContentSHA256}}, + {name: "presigned query only", presigned: true, query: hashA}, + {name: "presigned header only", presigned: true, header: []string{hashA}, want: []string{hashA}}, + {name: "presigned matching query and header", presigned: true, query: hashA, header: []string{hashA}, want: []string{hashA}}, + {name: "presigned unsigned query with forged header", presigned: true, query: unsignedPayload, header: []string{hashA}, want: []string{unsignedPayload}}, + {name: "presigned duplicate header", presigned: true, header: []string{hashA, hashB}, want: []string{hashA}}, + {name: "presigned query with empty header", presigned: true, query: hashA, header: []string{""}, want: []string{hashA}}, + } { + t.Run(tc.name, func(t *testing.T) { + target := "http://minio.local/bucket/object" + if tc.query != "" { + target += "?" + xhttp.AmzContentSha256 + "=" + tc.query + } + r := httptest.NewRequest(http.MethodPut, target, nil) + if tc.header != nil { + r.Header[xhttp.AmzContentSha256] = tc.header + } + if tc.presigned { + presignBoundaryRequest(t, r, UTCNow(), []string{"host"}, globalActiveCred) + } else { + r.Header.Set(xhttp.Authorization, signV4Algorithm+" Credential=x/20260910/us-east-1/s3/aws4_request, SignedHeaders=host, Signature=x") + r.Form = r.URL.Query() + } + got, ok := getConditionValues(r, "", globalActiveCred)[xhttp.AmzContentSha256] + if ok != (tc.want != nil) || !slices.Equal(got, tc.want) { + t.Fatalf("policy value = %v (present=%v), want %v (present=%v)", got, ok, tc.want, tc.want != nil) + } + if tc.presigned { + if code := reqSignatureV4Verify(r, globalSite.Region(), serviceS3); code != ErrNone { + t.Fatalf("the presigned request must still verify: %s", niceError(code)) + } + } + }) + } +} + +func newSignatureBoundaryUser(t *testing.T, bucket, statements string) auth.Credentials { + t.Helper() + cred, err := auth.GetNewCredentials() + if err != nil { + t.Fatal(err) + } + if _, err := globalIAMSys.CreateUser(t.Context(), cred.AccessKey, madmin.AddOrUpdateUserReq{SecretKey: cred.SecretKey, Status: madmin.AccountEnabled}); err != nil { + t.Fatal(err) + } + p, err := policy.ParseConfig(strings.NewReader(fmt.Sprintf(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:PutObject","Resource":"arn:aws:s3:::%s/*"},%s]}`, bucket, statements))) + if err != nil { + t.Fatal(err) + } + name := "signature-boundary-" + mustGetUUID() + if _, err := globalIAMSys.SetPolicy(t.Context(), name, *p); err != nil { + t.Fatal(err) + } + if _, err := globalIAMSys.PolicyDBSet(t.Context(), cred.AccessKey, name, regUser, false); err != nil { + t.Fatal(err) + } + return cred +} + +func TestAPIPresignedSignatureAgeBeforeAuthorization(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + endpoints: []string{"NewMultipart", "PutObjectPart", "PutObject"}, + objAPITest: func(obj ObjectLayer, instanceType, bucket string, router http.Handler, root auth.Credentials, t *testing.T) { + user := newSignatureBoundaryUser(t, bucket, fmt.Sprintf(`{"Effect":"Deny","Action":"s3:PutObject","Resource":"arn:aws:s3:::%s/*","Condition":{"NumericGreaterThan":{"s3:signatureAge":"60000"}}}`, bucket)) + initReq, err := newTestSignedRequestV4(http.MethodPost, getNewMultipartURL("", bucket, "multipart"), 0, nil, root.AccessKey, root.SecretKey, nil) + if err != nil { + t.Fatal(err) + } + initRec := httptest.NewRecorder() + router.ServeHTTP(initRec, initReq) + var upload InitiateMultipartUploadResponse + if initRec.Code != http.StatusOK || xml.Unmarshal(initRec.Body.Bytes(), &upload) != nil { + t.Fatalf("multipart initiation: %d %s", initRec.Code, initRec.Body.String()) + } + for _, operation := range []string{"put", "part"} { + for _, tc := range []struct { + name, header string + age time.Duration + want int + }{ + {name: "fresh", want: http.StatusOK}, + {name: "old without header", age: 10 * time.Minute, want: http.StatusForbidden}, + {name: "old forged header", age: 10 * time.Minute, header: "0", want: http.StatusForbidden}, + // Policy allows a fresh signature; the unsigned header then fails + // verification with the existing ErrUnsignedHeaders (HTTP 400). + {name: "fresh with unsigned header", header: "0", want: http.StatusBadRequest}, + } { + t.Run(instanceType+"/"+operation+"/"+tc.name, func(t *testing.T) { + target := getPutObjectURL("", bucket, "put-"+strings.ReplaceAll(tc.name, " ", "-")) + if operation == "part" { + target = getPutObjectPartURL("", bucket, "multipart", upload.UploadID, "1") + } + r, err := newTestRequest(http.MethodPut, target, 4, bytes.NewReader([]byte("body"))) + if err != nil { + t.Fatal(err) + } + r.Header.Del(xhttp.AmzContentSha256) + presignBoundaryRequest(t, r, UTCNow().Add(-tc.age), []string{"host"}, user) + if tc.header != "" { + r.Header.Set(forgedSignatureAgeHeader, tc.header) + } + rec := httptest.NewRecorder() + router.ServeHTTP(rec, r) + if rec.Code != tc.want { + t.Errorf("got %d %s, want %d", rec.Code, rec.Body.String(), tc.want) + } + }) + } + } + }, + }) +} + +func TestAPIPayloadHashPolicyMatchesVerifiedValue(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + endpoints: []string{"PutObject"}, + objAPITest: func(_ ObjectLayer, instanceType, bucket string, router http.Handler, _ auth.Credentials, t *testing.T) { + allowedHash := getSHA256Hash([]byte("expected")) + user := newSignatureBoundaryUser(t, bucket, fmt.Sprintf(`{"Effect":"Deny","Action":"s3:PutObject","Resource":"arn:aws:s3:::%s/*","Condition":{"StringNotEquals":{"s3:x-amz-content-sha256":"%s"}}}`, bucket, allowedHash)) + for _, kind := range []string{"signed control", "presigned control", "unsigned query forged header", "signed duplicate header", "presigned duplicate header"} { + t.Run(instanceType+"/"+kind, func(t *testing.T) { + control := strings.HasSuffix(kind, "control") + body := "modified" + if control { + body = "expected" + } + r, err := newTestRequest(http.MethodPut, getPutObjectURL("", bucket, strings.ReplaceAll(kind, " ", "-")), int64(len(body)), strings.NewReader(body)) + if err != nil { + t.Fatal(err) + } + if strings.Contains(kind, "duplicate") { + r.Header.Add(xhttp.AmzContentSha256, allowedHash) + } + if kind == "unsigned query forged header" { + q := r.URL.Query() + q.Set(xhttp.AmzContentSha256, unsignedPayload) + r.URL.RawQuery = q.Encode() + r.Header.Set(xhttp.AmzContentSha256, allowedHash) + } + if strings.HasPrefix(kind, "signed") { + if err := signRequestV4(r, user.AccessKey, user.SecretKey); err != nil { + t.Fatal(err) + } + } else { + presignBoundaryRequest(t, r, UTCNow(), []string{"host"}, user) + } + rec := httptest.NewRecorder() + router.ServeHTTP(rec, r) + if control && rec.Code != http.StatusOK { + t.Errorf("control rejected: %d %s", rec.Code, rec.Body.String()) + } + if !control && rec.Code < http.StatusBadRequest { + t.Errorf("payload-hash policy bypass returned %d %s", rec.Code, rec.Body.String()) + } + }) + } + }, + }) +} diff --git a/cmd/signature-v4-utils.go b/cmd/signature-v4-utils.go index 88ed7e9cd..f3f6815c6 100644 --- a/cmd/signature-v4-utils.go +++ b/cmd/signature-v4-utils.go @@ -211,7 +211,16 @@ func extractSignedHeaders(signedHeaders []string, r *http.Request) (http.Header, // `host` will not be found in the headers, can be found in r.Host. // but its always necessary that the list of signed headers containing host in it. val, ok := reqHeaders[http.CanonicalHeaderKey(header)] - if !ok { + if ok { + // Canonicalization comma-joins repeated header fields, so a signature + // over the single value "/src/a,tail" also verifies a request carrying + // ["/src/a", "tail"]. The copy handlers read only Header.Get, so that + // rewrite would copy a different source than the one signed. Reject a + // repeated x-amz-copy-source; a single value may still contain commas. + if len(val) > 1 && strings.EqualFold(header, strings.ToLower(xhttp.AmzCopySource)) { + return nil, ErrInvalidCopySource + } + } else { // try to set headers from Query String val, ok = reqQueries[header] } @@ -274,9 +283,8 @@ func signV4TrimAll(input string) string { // object the signing key can reach. // // Only headers actually sent by the client are inspected. Server-synthesized -// x-amz-* headers (e.g. x-amz-tagging derived from a request body, or the -// post-verification x-amz-signature-age scratch header) are set after signature -// verification and therefore never reach this walk. +// x-amz-* headers (e.g. x-amz-tagging derived from a request body) are set +// after signature verification and therefore never reach this walk. func checkUnsignedHeaders(signedHeadersMap http.Header, r *http.Request) APIErrorCode { // check headers that arrived on the request for k := range r.Header { @@ -294,14 +302,6 @@ func checkUnsignedHeaders(signedHeadersMap http.Header, r *http.Request) APIErro if strings.EqualFold(k, xhttp.AmzContentSha256) { continue } - // X-Amz-Signature-Age is an internal scratch header written by the - // presigned verifier itself, after this check, purely so bucket-policy - // evaluation can expose s3:signatureAge. It is never sent or signed by a - // client, and exempting it keeps signature verification idempotent when - // the same request is verified more than once. - if strings.EqualFold(k, xhttp.AmzSignatureAge) { - continue - } // The header must be a member of the signed-headers list. Testing // membership (not value equality) is essential: an unsigned header whose // first value is empty would otherwise compare equal to the empty string diff --git a/cmd/signature-v4-utils_test.go b/cmd/signature-v4-utils_test.go index d957908b9..8cccf6116 100644 --- a/cmd/signature-v4-utils_test.go +++ b/cmd/signature-v4-utils_test.go @@ -469,15 +469,16 @@ func TestCheckUnsignedHeaders(t *testing.T) { t.Fatalf("unsigned x-amz-content-sha256 must be exempt: expected %d, got %d", ErrNone, errCode) } - // X-Amz-Signature-Age is the presigned verifier's own scratch header, - // written after this check. Exempting it keeps verification idempotent when - // the same request object is verified more than once. + // X-Amz-Signature-Age was once a scratch header the presigned verifier wrote + // back and this check exempted. s3:signatureAge is now derived from the + // signed date, so a client that sends the header is sending an ordinary + // unsigned x-amz-* header and must be rejected like any other. r, err = http.NewRequest(http.MethodPut, "http://play.min.io:9000", nil) if err != nil { t.Fatal("Unable to create http.Request :", err) } - r.Header.Set(xhttp.AmzSignatureAge, "1234") - if errCode = checkUnsignedHeaders(signedHeadersMap, r); errCode != ErrNone { - t.Fatalf("internal x-amz-signature-age must be exempt: expected %d, got %d", ErrNone, errCode) + r.Header.Set("X-Amz-Signature-Age", "1234") + if errCode = checkUnsignedHeaders(signedHeadersMap, r); errCode != ErrUnsignedHeaders { + t.Fatalf("unsigned x-amz-signature-age must be rejected: expected %d, got %d", ErrUnsignedHeaders, errCode) } } diff --git a/cmd/signature-v4.go b/cmd/signature-v4.go index f204cd33c..d0125fb1c 100644 --- a/cmd/signature-v4.go +++ b/cmd/signature-v4.go @@ -336,8 +336,6 @@ func doesPresignedSignatureMatch(hashedPayload string, r *http.Request, region s return ErrSignatureDoesNotMatch } - r.Header.Set(xhttp.AmzSignatureAge, strconv.FormatInt(UTCNow().Sub(pSignValues.Date).Milliseconds(), 10)) - return ErrNone } diff --git a/cmd/signature-v4_test.go b/cmd/signature-v4_test.go index 72129940d..4f8b6dc07 100644 --- a/cmd/signature-v4_test.go +++ b/cmd/signature-v4_test.go @@ -25,8 +25,6 @@ import ( "os" "testing" "time" - - xhttp "github.com/minio/minio/internal/http" ) func niceError(code APIErrorCode) string { @@ -317,11 +315,11 @@ func TestDoesPresignedSignatureMatch(t *testing.T) { } // TestPresignedVerifyIdempotent guards against a regression where verifying the -// same presigned request twice began to fail. doesPresignedSignatureMatch -// writes an internal x-amz-signature-age header after validating the signature; -// the unsigned-header check must exempt that scratch header (and an unsigned -// x-amz-content-sha256 the client may carry) so a second verification of the -// same *http.Request still succeeds. +// same presigned request twice began to fail. The verifier must not write +// anything back to the request (it once recorded an internal +// x-amz-signature-age header that the unsigned-header check then had to +// exempt), and an unsigned x-amz-content-sha256 the client may carry stays +// exempt, so a second verification of the same *http.Request still succeeds. func TestPresignedVerifyIdempotent(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) defer cancel() @@ -350,7 +348,9 @@ func TestPresignedVerifyIdempotent(t *testing.T) { t.Fatalf("first verification: expected ErrNone, got %s", niceError(got)) } if got := reqSignatureV4Verify(req, globalSite.Region(), serviceS3); got != ErrNone { - t.Fatalf("second verification of the same request: expected ErrNone, got %s (x-amz-signature-age=%q)", - niceError(got), req.Header.Get(xhttp.AmzSignatureAge)) + t.Fatalf("second verification of the same request: expected ErrNone, got %s", niceError(got)) + } + if _, ok := req.Header["X-Amz-Signature-Age"]; ok { + t.Fatal("the verifier wrote a scratch header back to the request") } } diff --git a/go.mod b/go.mod index a9ff1166e..9f3618526 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.27.1 // Console and MC retain their historical module paths for best-effort upstream // compatibility. Pin the maintained PGSTY implementations used by SILO. -replace github.com/minio/console => github.com/pgsty/silo-console v0.0.0-20260910093545-6a0b31b5ade2 +replace github.com/minio/console => github.com/pgsty/silo-console v0.0.0-20260911085047-638eefd7aece replace github.com/minio/mc => github.com/pgsty/mc v0.0.0-20260910093317-e6a60edf0952 @@ -92,7 +92,7 @@ require ( github.com/prometheus/common v0.71.0 github.com/prometheus/procfs v0.22.0 github.com/puzpuzpuz/xsync/v3 v3.5.1 - github.com/rabbitmq/amqp091-go v1.10.0 + github.com/rabbitmq/amqp091-go v1.14.0 github.com/rcrowley/go-metrics v0.0.0-20250401214520-65e299d6c5c9 github.com/rs/cors v1.11.1 github.com/secure-io/sio-go v0.3.1 diff --git a/go.sum b/go.sum index d94781d25..bbf0396f1 100644 --- a/go.sum +++ b/go.sum @@ -547,8 +547,8 @@ github.com/pborman/getopt v0.0.0-20170112200414-7148bc3a4c30/go.mod h1:85jBQOZwp github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic= github.com/pgsty/mc v0.0.0-20260910093317-e6a60edf0952 h1:b6jpBuZWUiZhBox64eF1cmNL4HWGwdSk/LVXNc3DBQ0= github.com/pgsty/mc v0.0.0-20260910093317-e6a60edf0952/go.mod h1:VsGNEmditljwBgmYKy7X+//dXLN8gEfSPR2m4lcMPmo= -github.com/pgsty/silo-console v0.0.0-20260910093545-6a0b31b5ade2 h1:v/AXKa/UZkDheLLGrpKmyu8bVHo11dVrDHdbDvjX1/M= -github.com/pgsty/silo-console v0.0.0-20260910093545-6a0b31b5ade2/go.mod h1:bCyOnZactQajLOqlgCn0lHwQWMNylvP6MUOqqLako00= +github.com/pgsty/silo-console v0.0.0-20260911085047-638eefd7aece h1:1QGCeVCRV4TzDLww9IFfRT70YOI8r/X88pRo+KILvZ4= +github.com/pgsty/silo-console v0.0.0-20260911085047-638eefd7aece/go.mod h1:bCyOnZactQajLOqlgCn0lHwQWMNylvP6MUOqqLako00= github.com/pgsty/silo-pkg/v3 v3.13.4-0.20260910091716-2d8fd3cbbf07 h1:IKm2AyPsvuL4NyniK3ixqqQz1CIargmhyCkL2eZvlhA= github.com/pgsty/silo-pkg/v3 v3.13.4-0.20260910091716-2d8fd3cbbf07/go.mod h1:1JdcUcM+TRXObb9QrC8FWhav0RKk6/u8RT2RgimfxRQ= github.com/philhofer/fwd v1.2.0 h1:e6DnBTl7vGY+Gz322/ASL4Gyp1FspeMvx1RNDoToZuM= @@ -590,8 +590,8 @@ github.com/prometheus/prometheus v0.314.0 h1:YjsimqsIi6/mOtzZcrPEYUALO6zpfaht9O5 github.com/prometheus/prometheus v0.314.0/go.mod h1:zjg3pMTAkY0/JG8jy/h8/YgSQUVB+aCXMhUqN6l64jg= github.com/puzpuzpuz/xsync/v3 v3.5.1 h1:GJYJZwO6IdxN/IKbneznS6yPkVC+c3zyY/j19c++5Fg= github.com/puzpuzpuz/xsync/v3 v3.5.1/go.mod h1:VjzYrABPabuM4KyBh1Ftq6u8nhwY5tBPKP9jpmh0nnA= -github.com/rabbitmq/amqp091-go v1.10.0 h1:STpn5XsHlHGcecLmMFCtg7mqq0RnD+zFr4uzukfVhBw= -github.com/rabbitmq/amqp091-go v1.10.0/go.mod h1:Hy4jKW5kQART1u+JkDTF9YYOQUHXqMuhrgxOEeS7G4o= +github.com/rabbitmq/amqp091-go v1.14.0 h1:RSaT7aOKt/OrkVUyswPDW29lnRz9psuGmfZFBmLqLek= +github.com/rabbitmq/amqp091-go v1.14.0/go.mod h1:Hy4jKW5kQART1u+JkDTF9YYOQUHXqMuhrgxOEeS7G4o= github.com/rcrowley/go-metrics v0.0.0-20250401214520-65e299d6c5c9 h1:bsUq1dX0N8AOIL7EB/X911+m4EHsnWEHeJ0c+3TTBrg= github.com/rcrowley/go-metrics v0.0.0-20250401214520-65e299d6c5c9/go.mod h1:bCqnVzQkZxMG4s8nGwiZ5l3QUCyqpo9Y+/ZMZ9VjZe4= github.com/rivo/uniseg v0.1.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc= diff --git a/internal/event/target/amqp_test.go b/internal/event/target/amqp_test.go new file mode 100644 index 000000000..596de201f --- /dev/null +++ b/internal/event/target/amqp_test.go @@ -0,0 +1,77 @@ +// Copyright (c) 2026 PGSTY +// SPDX-License-Identifier: AGPL-3.0-only + +package target + +import ( + "context" + "errors" + "fmt" + "io" + "net" + "testing" + "time" + + "github.com/rabbitmq/amqp091-go" +) + +// A broker must not make the notification client allocate or read an oversized +// frame, including before connection.tune negotiates the frame size limit. +func TestAMQPRejectsOversizedHandshakeFrame(t *testing.T) { + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { listener.Close() }) + + uri, err := amqp091.ParseURI("amqp://" + listener.Addr().String()) + if err != nil { + t.Fatal(err) + } + target, err := NewAMQPTarget("oversized-frame", AMQPArgs{Enable: true, URL: uri}, + func(context.Context, error, string, ...any) {}) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { target.Close() }) + + brokerDone := make(chan error, 1) + go func() { + brokerDone <- func() error { + conn, err := listener.Accept() + if err != nil { + return err + } + defer conn.Close() + if err := conn.SetDeadline(time.Now().Add(5 * time.Second)); err != nil { + return err + } + var protocol [8]byte + if _, err := io.ReadFull(conn, protocol[:]); err != nil { + return err + } + if protocol != [8]byte{'A', 'M', 'Q', 'P', 0, 0, 9, 1} { + return fmt.Errorf("unexpected AMQP protocol header: %x", protocol) + } + // A method frame declaring an 8 KiB payload exceeds the 4 KiB + // pre-negotiation limit. Send only its header: rejection must + // happen without allocating or waiting for the declared body. + if _, err := conn.Write([]byte{1, 0, 0, 0, 0, 0x20, 0}); err != nil { + return err + } + var response [1]byte + if n, err := conn.Read(response[:]); n != 0 || !errors.Is(err, io.EOF) { + return fmt.Errorf("client did not close after oversized frame header: read %d bytes, error %v", n, err) + } + return nil + }() + }() + + active, err := target.IsActive() + if active || err == nil { + t.Errorf("oversized handshake frame accepted: active=%v, error=%v", active, err) + } + if err := <-brokerDone; err != nil { + t.Fatal(err) + } +} diff --git a/internal/http/headers.go b/internal/http/headers.go index 93e55264f..9195e122e 100644 --- a/internal/http/headers.go +++ b/internal/http/headers.go @@ -132,11 +132,6 @@ const ( AmzMaxParts = "X-Amz-Max-Parts" AmzPartNumberMarker = "X-Amz-Part-Number-Marker" - // AmzSignatureAge is an internal scratch header the presigned verifier - // writes after validating the signature so that bucket-policy evaluation can - // expose s3:signatureAge. It is never sent or signed by a client. - AmzSignatureAge = "X-Amz-Signature-Age" - // Constants used for GetObjectAttributes and GetObjectVersionAttributes AmzObjectAttributes = "X-Amz-Object-Attributes"