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/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/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"