Require signed copy source headers in SigV4 requests

cmd/object-handlers_test.go

@@ -1619,14 +1619,17 @@ 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: <ERROR> %v", err)
 		}
 
 		// "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: <ERROR> %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: <ERROR> %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)

cmd/signature-v4-copy_test.go

@@ -0,0 +1,243 @@
+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 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,
+		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 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)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, _, _, _, code := calculateSeedSignature(req); code != ErrNone {
+		t.Fatalf("normal streaming signature: %v", 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)
+		}
+	}
+}
+
+// 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)
+						}
+					})
+				}
+			}
+		}
+	}
+}

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.