From 129d004de9bb06f1b9d887677dd690e9ae6225c5 Mon Sep 17 00:00:00 2001 From: zhijian Date: Thu, 10 Sep 2026 11:24:04 +0800 Subject: [PATCH 1/2] Require signed copy source headers in SigV4 requests --- cmd/object-handlers_test.go | 21 ++-- cmd/signature-v4-copy_test.go | 194 ++++++++++++++++++++++++++++++++++ cmd/signature-v4-utils.go | 8 ++ 3 files changed, 217 insertions(+), 6 deletions(-) create mode 100644 cmd/signature-v4-copy_test.go diff --git a/cmd/object-handlers_test.go b/cmd/object-handlers_test.go index 4178529a21fce..1695315b6b460 100644 --- a/cmd/object-handlers_test.go +++ b/cmd/object-handlers_test.go @@ -1619,7 +1619,7 @@ func testAPICopyObjectPartHandlerSanity(obj ObjectLayer, instanceType, bucketNam // construct HTTP request for copy object. var req *http.Request - req, err = newTestSignedRequestV4(http.MethodPut, cpPartURL, 0, nil, credentials.AccessKey, credentials.SecretKey, nil) + req, err = newTestRequest(http.MethodPut, cpPartURL, 0, nil) if err != nil { t.Fatalf("Test failed to create HTTP request for copy object part: %v", err) } @@ -1627,6 +1627,9 @@ func testAPICopyObjectPartHandlerSanity(obj ObjectLayer, instanceType, bucketNam // "X-Amz-Copy-Source" header contains the information about the source bucket and the object to copied. req.Header.Set("X-Amz-Copy-Source", url.QueryEscape(pathJoin(bucketName, objectName))) req.Header.Set("X-Amz-Copy-Source-Range", fmt.Sprintf("bytes=%d-%d", a, b)) + if err = signRequestV4(req, credentials.AccessKey, credentials.SecretKey); err != nil { + t.Fatal(err) + } // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. // Call the ServeHTTP to execute the handler, `func (api objectAPIHandlers) CopyObjectHandler` handles the request. @@ -1924,11 +1927,11 @@ func testAPICopyObjectPartHandler(obj ObjectLayer, instanceType, bucketName stri rec := httptest.NewRecorder() if !testCase.invalidPartNumber || !testCase.maximumPartNumber { // construct HTTP request for copy object. - req, err = newTestSignedRequestV4(http.MethodPut, getCopyObjectPartURL("", testCase.bucketName, testObject, testCase.uploadID, "1"), 0, nil, testCase.accessKey, testCase.secretKey, nil) + req, err = newTestRequest(http.MethodPut, getCopyObjectPartURL("", testCase.bucketName, testObject, testCase.uploadID, "1"), 0, nil) } else if testCase.invalidPartNumber { - req, err = newTestSignedRequestV4(http.MethodPut, getCopyObjectPartURL("", testCase.bucketName, testObject, testCase.uploadID, "abc"), 0, nil, testCase.accessKey, testCase.secretKey, nil) + req, err = newTestRequest(http.MethodPut, getCopyObjectPartURL("", testCase.bucketName, testObject, testCase.uploadID, "abc"), 0, nil) } else if testCase.maximumPartNumber { - req, err = newTestSignedRequestV4(http.MethodPut, getCopyObjectPartURL("", testCase.bucketName, testObject, testCase.uploadID, "99999"), 0, nil, testCase.accessKey, testCase.secretKey, nil) + req, err = newTestRequest(http.MethodPut, getCopyObjectPartURL("", testCase.bucketName, testObject, testCase.uploadID, "99999"), 0, nil) } if err != nil { t.Fatalf("Test %d: Failed to create HTTP request for copy Object: %v", i+1, err) @@ -1941,6 +1944,9 @@ func testAPICopyObjectPartHandler(obj ObjectLayer, instanceType, bucketName stri if testCase.copySourceRange != "" { req.Header.Set("X-Amz-Copy-Source-Range", testCase.copySourceRange) } + if err = signRequestV4(req, testCase.accessKey, testCase.secretKey); err != nil { + t.Fatal(err) + } // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. // Call the ServeHTTP to execute the handler, `func (api objectAPIHandlers) CopyObjectHandler` handles the request. @@ -2301,8 +2307,8 @@ func testAPICopyObjectHandler(obj ObjectLayer, instanceType, bucketName string, // initialize HTTP NewRecorder, this records any mutations to response writer inside the handler. rec := httptest.NewRecorder() // construct HTTP request for copy object. - req, err = newTestSignedRequestV4(http.MethodPut, getCopyObjectURL("", testCase.bucketName, testCase.newObjectName), - 0, nil, testCase.accessKey, testCase.secretKey, nil) + req, err = newTestRequest(http.MethodPut, getCopyObjectURL("", testCase.bucketName, testCase.newObjectName), + 0, nil) if err != nil { t.Fatalf("Test %d: Failed to create HTTP request for copy Object: %v", i, err) @@ -2330,6 +2336,9 @@ func testAPICopyObjectHandler(obj ObjectLayer, instanceType, bucketName string, if testCase.metadataGarbage { req.Header.Set("X-Amz-Metadata-Directive", "Unknown") } + if err = signRequestV4(req, testCase.accessKey, testCase.secretKey); err != nil { + t.Fatal(err) + } // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. // Call the ServeHTTP to execute the handler, `func (api objectAPIHandlers) CopyObjectHandler` handles the request. apiRouter.ServeHTTP(rec, req) diff --git a/cmd/signature-v4-copy_test.go b/cmd/signature-v4-copy_test.go new file mode 100644 index 0000000000000..d8196f7f81f82 --- /dev/null +++ b/cmd/signature-v4-copy_test.go @@ -0,0 +1,194 @@ +package cmd + +import ( + "context" + "encoding/xml" + "io" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + + "github.com/minio/minio-go/v7/pkg/signer" + xhttp "github.com/minio/minio/cmd/http" +) + +func TestExtractSignedCopyHeaders(t *testing.T) { + for _, header := range []string{ + xhttp.AmzCopySource, + xhttp.AmzCopySourceRange, + xhttp.AmzCopySourceIfMatch, + xhttp.AmzCopySourceIfNoneMatch, + xhttp.AmzCopySourceIfModifiedSince, + xhttp.AmzCopySourceIfUnmodifiedSince, + "X-Amz-Copy-Source-Server-Side-Encryption-Customer-Key", + } { + t.Run(header, func(t *testing.T) { + for _, value := range []string{"/source/secret", ""} { + req := httptest.NewRequest(http.MethodPut, "http://localhost/destination/object", nil) + req.Header.Set(header, value) + if _, code := extractSignedHeaders([]string{"host"}, req); code != ErrUnsignedHeaders { + t.Errorf("unsigned %s = %q: got %v, want ErrUnsignedHeaders", header, value, code) + } + if _, code := extractSignedHeaders([]string{"host", strings.ToLower(header)}, req); code != ErrNone { + t.Errorf("signed %s = %q: got %v, want ErrNone", header, value, code) + } + } + }) + } + req := httptest.NewRequest(http.MethodPut, "http://localhost/destination/object", nil) + req.Header.Set(xhttp.AmzCopySource, "/source/secret") + req.Header.Set(xhttp.AmzCopySourceRange, "bytes=0-1") + if _, code := extractSignedHeaders([]string{"host", "x-amz-copy-source"}, req); code != ErrUnsignedHeaders { + t.Errorf("unsigned range on signed copy: got %v, want ErrUnsignedHeaders", code) + } +} + +func TestStreamingSignedCopyHeaders(t *testing.T) { + cred := globalActiveCred + req, err := newTestStreamingSignedRequest(http.MethodPut, "http://localhost/destination/object", 4, 4, strings.NewReader("data"), cred.AccessKey, cred.SecretKey) + if err != nil { + t.Fatal(err) + } + if _, _, _, _, code := calculateSeedSignature(req); code != ErrNone { + t.Fatalf("normal streaming signature: %v", code) + } + req.Header.Set(xhttp.AmzCopySource, "/source/secret") + if _, _, _, _, code := calculateSeedSignature(req); code != ErrUnsignedHeaders { + t.Errorf("unsigned copy header on streaming request: got %v, want ErrUnsignedHeaders", code) + } +} + +// Exercise real PUT/copy routing with the existing filesystem backend and SDK signer. +func TestSigV4CopyHeaderAuthorization(t *testing.T) { + resetTestGlobals() + defer resetTestGlobals() + ctx := context.Background() + obj, dir, err := prepareFS() + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + defer obj.Shutdown(ctx) + if err = newTestConfig(globalMinioDefaultRegion, obj); err != nil { + t.Fatal(err) + } + bucket, _, err := initAPIHandlerTest(obj, []string{"CopyObject"}) + if err != nil { + t.Fatal(err) + } + router := initTestAPIEndPoints(obj, nil) + otherBucket := getRandomBucketName() + if err = obj.MakeBucketWithLocation(ctx, otherBucket, BucketOptions{}); err != nil { + t.Fatal(err) + } + const original = "original upload" + const secret = "source secret" + put := func(t *testing.T, bucket, key, data string) { + t.Helper() + if _, err := obj.PutObject(ctx, bucket, key, mustGetPutObjReader(t, strings.NewReader(data), int64(len(data)), "", ""), ObjectOptions{}); err != nil { + t.Fatal(err) + } + } + for _, sourceBucket := range []string{bucket, otherBucket} { + put(t, sourceBucket, "secret", secret) + for _, presigned := range []bool{false, true} { + for _, multipart := range []bool{false, true} { + for _, scenario := range []string{"upload", "unsigned-copy", "signed-copy", "tampered-copy"} { + name := sourceBucket + "/authorization/" + if presigned { + name = sourceBucket + "/presigned/" + } + if multipart { + name += "part/" + } + name += scenario + t.Run(name, func(t *testing.T) { + const target = "target" + put(t, bucket, target, original) + requestURL := "http://localhost/" + bucket + "/" + target + uploadID := "" + if multipart { + uploadID, err = obj.NewMultipartUpload(ctx, bucket, target, ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + defer obj.AbortMultipartUpload(ctx, bucket, target, uploadID, ObjectOptions{}) + if _, err := obj.PutObjectPart(ctx, bucket, target, uploadID, 1, mustGetPutObjReader(t, strings.NewReader(original), int64(len(original)), "", ""), ObjectOptions{}); err != nil { + t.Fatal(err) + } + requestURL = getPutObjectPartURL("http://localhost", bucket, target, uploadID, "1") + } + body := "" + if scenario == "upload" { + body = "new upload" + } + req, err := http.NewRequest(http.MethodPut, requestURL, strings.NewReader(body)) + if err != nil { + t.Fatal(err) + } + if scenario == "signed-copy" || scenario == "tampered-copy" { + req.Header.Set(xhttp.AmzCopySource, "/"+sourceBucket+"/secret") + } + cred := globalActiveCred + if presigned { + req = signer.PreSignV4(*req, cred.AccessKey, cred.SecretKey, "", globalServerRegion, 60) + } else { + req.Header.Set(xhttp.AmzContentSha256, unsignedPayload) + if err = signRequestV4(req, cred.AccessKey, cred.SecretKey); err != nil { + t.Fatal(err) + } + } + wantCode := ErrNone + wantData := body + switch scenario { + case "unsigned-copy": + req.Header.Set(xhttp.AmzCopySource, "/"+sourceBucket+"/secret") + wantCode, wantData = ErrUnsignedHeaders, original + case "tampered-copy": + req.Header.Set(xhttp.AmzCopySource, "/"+sourceBucket+"/target") + wantCode, wantData = ErrSignatureDoesNotMatch, original + case "signed-copy": + wantData = secret + } + rec := httptest.NewRecorder() + req.RequestURI = req.URL.RequestURI() + router.ServeHTTP(rec, req) + wantStatus := http.StatusOK + if wantCode != ErrNone { + wantStatus = getAPIError(wantCode).HTTPStatusCode + var response APIErrorResponse + if err := xml.Unmarshal(rec.Body.Bytes(), &response); err != nil { + t.Errorf("decode rejection: %v; body: %s", err, rec.Body) + } else if response.Code != getAPIError(wantCode).Code { + t.Errorf("error code = %s, want %s", response.Code, getAPIError(wantCode).Code) + } + } + if rec.Code != wantStatus { + t.Errorf("HTTP %d, want %d: %s", rec.Code, wantStatus, rec.Body) + } + if multipart { + parts, err := obj.ListObjectParts(ctx, bucket, target, uploadID, 0, 10, ObjectOptions{}) + if err != nil || len(parts.Parts) != 1 { + t.Fatalf("list parts: %v, parts: %v", err, parts.Parts) + } + if _, err = obj.CompleteMultipartUpload(ctx, bucket, target, uploadID, []CompletePart{{PartNumber: 1, ETag: parts.Parts[0].ETag}}, ObjectOptions{}); err != nil { + t.Fatal(err) + } + } + reader, err := obj.GetObjectNInfo(ctx, bucket, target, nil, nil, readLock, ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + defer reader.Close() + data, err := io.ReadAll(reader) + if err != nil || string(data) != wantData { + t.Errorf("target = %q, want %q; error: %v", data, wantData, err) + } + }) + } + } + } + } +} diff --git a/cmd/signature-v4-utils.go b/cmd/signature-v4-utils.go index 90f53f955c0c1..aa7c29cac505c 100644 --- a/cmd/signature-v4-utils.go +++ b/cmd/signature-v4-utils.go @@ -196,6 +196,14 @@ func extractSignedHeaders(signedHeaders []string, r *http.Request) (http.Header, return nil, ErrUnsignedHeaders } } + // Copy headers can turn an upload into a read of another object, or change + // the source range and conditions. Require them to be covered by the signature. + for header := range reqHeaders { + header = strings.ToLower(header) + if strings.HasPrefix(header, "x-amz-copy-source") && !contains(signedHeaders, header) { + return nil, ErrUnsignedHeaders + } + } return extractedSignedHeaders, ErrNone } From 643115d1db3cc1f9275f3654c7ec8c228289ef67 Mon Sep 17 00:00:00 2001 From: zhijian Date: Mon, 14 Sep 2026 14:31:15 +0800 Subject: [PATCH 2/2] Require signatures for all x-amz headers --- cmd/signature-v4-copy_test.go | 57 ++++++++++++++++++++++++++++++++--- cmd/signature-v4-utils.go | 16 +++++----- 2 files changed, 61 insertions(+), 12 deletions(-) diff --git a/cmd/signature-v4-copy_test.go b/cmd/signature-v4-copy_test.go index d8196f7f81f82..161bcc954d578 100644 --- a/cmd/signature-v4-copy_test.go +++ b/cmd/signature-v4-copy_test.go @@ -14,8 +14,15 @@ import ( xhttp "github.com/minio/minio/cmd/http" ) -func TestExtractSignedCopyHeaders(t *testing.T) { +func TestExtractSignedAmzHeaders(t *testing.T) { for _, header := range []string{ + xhttp.AmzDate, + xhttp.AmzSecurityToken, + xhttp.AmzMetadataDirective, + xhttp.AmzTagDirective, + "X-Amz-Meta-Test", + "X-Amz-Server-Side-Encryption", + "X-Amz-Content-Sha256-Extra", xhttp.AmzCopySource, xhttp.AmzCopySourceRange, xhttp.AmzCopySourceIfMatch, @@ -45,6 +52,45 @@ func TestExtractSignedCopyHeaders(t *testing.T) { } } +func TestSigV4AmzHeaderCompatibility(t *testing.T) { + cred := globalActiveCred + defer func() { globalActiveCred = cred }() + globalActiveCred.SessionToken = "test-session-token" + for _, mode := range []string{"authorization", "presigned"} { + t.Run(mode, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPut, "http://localhost/destination/object?x-amz-meta-query=value", nil) + req.Header.Set("X-Amz-Meta-Test", "signed metadata") + if mode == "presigned" { + req = signer.PreSignV4(*req, cred.AccessKey, cred.SecretKey, globalActiveCred.SessionToken, globalServerRegion, 60) + } else { + req = signer.SignV4(*req, cred.AccessKey, cred.SecretKey, globalActiveCred.SessionToken, globalServerRegion) + } + // The SDK uses UNSIGNED-PAYLOAD when this header is absent during signing. + req.Header.Set(xhttp.AmzContentSha256, unsignedPayload) + req.Header.Set("User-Agent", "unsigned user agent") + if code := reqSignatureV4Verify(req, globalServerRegion, serviceS3); code != ErrNone { + t.Fatalf("valid signature with unsigned payload-hash header: %v", code) + } + for _, test := range []struct { + name, header, value string + want APIErrorCode + }{ + {"unsigned-metadata-directive", xhttp.AmzMetadataDirective, "REPLACE", ErrUnsignedHeaders}, + {"tampered-metadata", "X-Amz-Meta-Test", "changed metadata", ErrSignatureDoesNotMatch}, + {"tampered-payload-hash", xhttp.AmzContentSha256, emptySHA256, ErrSignatureDoesNotMatch}, + } { + t.Run(test.name, func(t *testing.T) { + modified := req.Clone(req.Context()) + modified.Header.Set(test.header, test.value) + if code := reqSignatureV4Verify(modified, globalServerRegion, serviceS3); code != test.want { + t.Errorf("got %v, want %v", code, test.want) + } + }) + } + }) + } +} + func TestStreamingSignedCopyHeaders(t *testing.T) { cred := globalActiveCred req, err := newTestStreamingSignedRequest(http.MethodPut, "http://localhost/destination/object", 4, 4, strings.NewReader("data"), cred.AccessKey, cred.SecretKey) @@ -54,9 +100,12 @@ func TestStreamingSignedCopyHeaders(t *testing.T) { if _, _, _, _, code := calculateSeedSignature(req); code != ErrNone { t.Fatalf("normal streaming signature: %v", code) } - req.Header.Set(xhttp.AmzCopySource, "/source/secret") - if _, _, _, _, code := calculateSeedSignature(req); code != ErrUnsignedHeaders { - t.Errorf("unsigned copy header on streaming request: got %v, want ErrUnsignedHeaders", code) + for _, header := range []string{xhttp.AmzCopySource, "X-Amz-Meta-Test"} { + modified := req.Clone(req.Context()) + modified.Header.Set(header, "/source/secret") + if _, _, _, _, code := calculateSeedSignature(modified); code != ErrUnsignedHeaders { + t.Errorf("unsigned %s on streaming request: got %v, want ErrUnsignedHeaders", header, code) + } } } diff --git a/cmd/signature-v4-utils.go b/cmd/signature-v4-utils.go index aa7c29cac505c..c922f60b322f9 100644 --- a/cmd/signature-v4-utils.go +++ b/cmd/signature-v4-utils.go @@ -149,6 +149,14 @@ func extractSignedHeaders(signedHeaders []string, r *http.Request) (http.Header, if !contains(signedHeaders, "host") { return nil, ErrUnsignedHeaders } + // All x-amz- headers must be signed, except x-amz-content-sha256, + // whose value is already included as the canonical request's payload hash. + for header := range reqHeaders { + header = strings.ToLower(header) + if strings.HasPrefix(header, "x-amz-") && header != "x-amz-content-sha256" && !contains(signedHeaders, header) { + return nil, ErrUnsignedHeaders + } + } extractedSignedHeaders := make(http.Header) for _, header := range signedHeaders { // `host` will not be found in the headers, can be found in r.Host. @@ -196,14 +204,6 @@ func extractSignedHeaders(signedHeaders []string, r *http.Request) (http.Header, return nil, ErrUnsignedHeaders } } - // Copy headers can turn an upload into a read of another object, or change - // the source range and conditions. Require them to be covered by the signature. - for header := range reqHeaders { - header = strings.ToLower(header) - if strings.HasPrefix(header, "x-amz-copy-source") && !contains(signedHeaders, header) { - return nil, ErrUnsignedHeaders - } - } return extractedSignedHeaders, ErrNone }