From 577470214d901b68bc40e0b4f4db08feedfd9db8 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Sat, 30 May 2026 21:16:26 +0400 Subject: [PATCH] fix: enforce required SignedHeaders validation for SigV4 requests Validate required signed headers for both Authorization-header SigV4 requests and presigned URLs. The required signed header set is now `host` plus every incoming header with the `x-amz-` prefix. During request reconstruction, signed headers and explicitly ignored headers are copied into the generated request used for signature verification. If an incoming `x-amz-*` header is present but missing from the client-provided `SignedHeaders`, return `AccessDenied` with a `HeadersNotSigned` field. The `host` header remains part of the canonical request and signed header calculation. Previously, a client could sign a request without an S3 control header and then add that header after signing. For example, a presigned `PUT` URL could be generated with only `host` signed, then the actual request could include an unsigned `X-Amz-Tagging` or `X-Amz-Copy-Source` header. Because the verifier reconstructed the request only from `SignedHeaders`, that extra header was omitted from signature calculation and could pass authentication even though it changed the request semantics. This is now rejected with `AccessDenied`. Expose v4 helper methods for checking required and ignored headers, and update canonical header signing so ignored headers can still be included when a client explicitly lists them in `SignedHeaders`, while `Authorization` remains excluded from signature calculation. --- aws/signer/internal/v4/header_rules.go | 8 +- aws/signer/internal/v4/headers.go | 59 +------ aws/signer/internal/v4/headers_test.go | 88 +++++++++- aws/signer/v4/header_rules.go | 14 ++ aws/signer/v4/v4.go | 14 +- aws/signer/v4/v4_test.go | 18 +- s3api/utils/auth-reader.go | 2 +- s3api/utils/presign-auth-reader.go | 2 +- s3api/utils/signed_headers_test.go | 227 +++++++++++++++++++++++++ s3api/utils/utils.go | 45 +++-- s3err/headers-not-signed-error.go | 61 +++++++ tests/integration/group-tests.go | 8 + tests/integration/presigned_urls.go | 60 +++++++ tests/integration/sigv4_auth.go | 68 ++++++-- tests/integration/utils.go | 3 + 15 files changed, 577 insertions(+), 100 deletions(-) create mode 100644 aws/signer/v4/header_rules.go create mode 100644 s3api/utils/signed_headers_test.go create mode 100644 s3err/headers-not-signed-error.go diff --git a/aws/signer/internal/v4/header_rules.go b/aws/signer/internal/v4/header_rules.go index 5d931d18b..ea08c4e12 100644 --- a/aws/signer/internal/v4/header_rules.go +++ b/aws/signer/internal/v4/header_rules.go @@ -30,8 +30,12 @@ type MapRule map[string]struct{} // IsValid for the map Rule satisfies whether it exists in the map func (m MapRule) IsValid(value string) bool { - _, ok := m[value] - return ok + for key := range m { + if strings.EqualFold(key, value) { + return true + } + } + return false } // AllowList is a generic Rule for include listing diff --git a/aws/signer/internal/v4/headers.go b/aws/signer/internal/v4/headers.go index d15532325..cbd221071 100644 --- a/aws/signer/internal/v4/headers.go +++ b/aws/signer/internal/v4/headers.go @@ -4,65 +4,24 @@ package v4 var IgnoredHeaders = Rules{ ExcludeList{ MapRule{ - "Authorization": struct{}{}, - // some clients use user-agent in signed headers - // "User-Agent": struct{}{}, - "X-Amzn-Trace-Id": struct{}{}, - // Expect might appear in signed headers - // "Expect": struct{}{}, + "Authorization": struct{}{}, + "User-Agent": struct{}{}, + "X-Amzn-Trace-Id": struct{}{}, + "Expect": struct{}{}, + "Transfer-Encoding": struct{}{}, }, }, } -// RequiredSignedHeaders is a allow list for Build canonical headers. +// RequiredSignedHeaders are request headers that must be part of SignedHeaders +// whenever they are present on the request. var RequiredSignedHeaders = Rules{ AllowList{ MapRule{ - "Cache-Control": struct{}{}, - "Content-Disposition": struct{}{}, - "Content-Encoding": struct{}{}, - "Content-Language": struct{}{}, - "Content-Md5": struct{}{}, - "Content-Type": struct{}{}, - "Expires": struct{}{}, - "If-Match": struct{}{}, - "If-Modified-Since": struct{}{}, - "If-None-Match": struct{}{}, - "If-Unmodified-Since": struct{}{}, - "Range": struct{}{}, - "X-Amz-Acl": struct{}{}, - "X-Amz-Copy-Source": struct{}{}, - "X-Amz-Copy-Source-If-Match": struct{}{}, - "X-Amz-Copy-Source-If-Modified-Since": struct{}{}, - "X-Amz-Copy-Source-If-None-Match": struct{}{}, - "X-Amz-Copy-Source-If-Unmodified-Since": struct{}{}, - "X-Amz-Copy-Source-Range": struct{}{}, - "X-Amz-Copy-Source-Server-Side-Encryption-Customer-Algorithm": struct{}{}, - "X-Amz-Copy-Source-Server-Side-Encryption-Customer-Key": struct{}{}, - "X-Amz-Copy-Source-Server-Side-Encryption-Customer-Key-Md5": struct{}{}, - "X-Amz-Expected-Bucket-Owner": struct{}{}, - "X-Amz-Grant-Full-control": struct{}{}, - "X-Amz-Grant-Read": struct{}{}, - "X-Amz-Grant-Read-Acp": struct{}{}, - "X-Amz-Grant-Write": struct{}{}, - "X-Amz-Grant-Write-Acp": struct{}{}, - "X-Amz-Metadata-Directive": struct{}{}, - "X-Amz-Mfa": struct{}{}, - "X-Amz-Request-Payer": struct{}{}, - "X-Amz-Server-Side-Encryption": struct{}{}, - "X-Amz-Server-Side-Encryption-Aws-Kms-Key-Id": struct{}{}, - "X-Amz-Server-Side-Encryption-Context": struct{}{}, - "X-Amz-Server-Side-Encryption-Customer-Algorithm": struct{}{}, - "X-Amz-Server-Side-Encryption-Customer-Key": struct{}{}, - "X-Amz-Server-Side-Encryption-Customer-Key-Md5": struct{}{}, - "X-Amz-Storage-Class": struct{}{}, - "X-Amz-Website-Redirect-Location": struct{}{}, - "X-Amz-Content-Sha256": struct{}{}, - "X-Amz-Tagging": struct{}{}, + "Host": struct{}{}, }, }, - Patterns{"X-Amz-Object-Lock-"}, - Patterns{"X-Amz-Meta-"}, + Patterns{"X-Amz-"}, } // AllowedQueryHoisting is a allowed list for Build query headers. The boolean value diff --git a/aws/signer/internal/v4/headers_test.go b/aws/signer/internal/v4/headers_test.go index 2f4ad2e3f..4484c2940 100644 --- a/aws/signer/internal/v4/headers_test.go +++ b/aws/signer/internal/v4/headers_test.go @@ -17,7 +17,19 @@ func TestAllowedQueryHoisting(t *testing.T) { }, "another header": { Header: "X-Amz-SomeOtherHeader", - ExpectHoist: true, + ExpectHoist: false, + }, + "lowercase amz header": { + Header: "x-amz-someotherheader", + ExpectHoist: false, + }, + "mixed case amz header": { + Header: "x-AmZ-someotherheader", + ExpectHoist: false, + }, + "non-amz content header": { + Header: "Content-Type", + ExpectHoist: false, }, "non X-AMZ header": { Header: "X-SomeOtherHeader", @@ -34,6 +46,62 @@ func TestAllowedQueryHoisting(t *testing.T) { } } +func TestRequiredSignedHeaders(t *testing.T) { + cases := map[string]struct { + Header string + ExpectRequired bool + }{ + "known content header": { + Header: "Content-Type", + ExpectRequired: false, + }, + "known content header lowercase": { + Header: "content-type", + ExpectRequired: false, + }, + "known conditional header": { + Header: "If-Match", + ExpectRequired: false, + }, + "range header": { + Header: "Range", + ExpectRequired: false, + }, + "content md5 header": { + Header: "Content-Md5", + ExpectRequired: false, + }, + "arbitrary amz header": { + Header: "X-Amz-SomeOtherHeader", + ExpectRequired: true, + }, + "arbitrary amz header lowercase": { + Header: "x-amz-someotherheader", + ExpectRequired: true, + }, + "object-lock amz header": { + Header: "X-Amz-Object-Lock-Mode", + ExpectRequired: true, + }, + "metadata amz header": { + Header: "X-Amz-Meta-SomeName", + ExpectRequired: true, + }, + "non-amz custom header": { + Header: "X-SomeOtherHeader", + ExpectRequired: false, + }, + } + + for name, c := range cases { + t.Run(name, func(t *testing.T) { + if e, a := c.ExpectRequired, RequiredSignedHeaders.IsValid(c.Header); e != a { + t.Errorf("expect required %v, was %v", e, a) + } + }) + } +} + func TestIgnoredHeaders(t *testing.T) { cases := map[string]struct { Header string @@ -41,12 +109,28 @@ func TestIgnoredHeaders(t *testing.T) { }{ "expect": { Header: "Expect", - ExpectIgnored: false, + ExpectIgnored: true, + }, + "user-agent": { + Header: "User-Agent", + ExpectIgnored: true, + }, + "transfer-encoding": { + Header: "Transfer-Encoding", + ExpectIgnored: true, }, "authorization": { Header: "Authorization", ExpectIgnored: true, }, + "authorization lowercase": { + Header: "authorization", + ExpectIgnored: true, + }, + "trace id lowercase": { + Header: "x-amzn-trace-id", + ExpectIgnored: true, + }, "X-AMZ header": { Header: "X-Amz-Content-Sha256", ExpectIgnored: false, diff --git a/aws/signer/v4/header_rules.go b/aws/signer/v4/header_rules.go new file mode 100644 index 000000000..dec685b53 --- /dev/null +++ b/aws/signer/v4/header_rules.go @@ -0,0 +1,14 @@ +package v4 + +import v4Internal "github.com/versity/versitygw/aws/signer/internal/v4" + +// IsRequiredSignedHeader reports whether a header must be signed when it is +// present on an incoming request. +func IsRequiredSignedHeader(header string) bool { + return v4Internal.RequiredSignedHeaders.IsValid(header) +} + +// IsIgnoredHeader reports whether a header is normally excluded from signing. +func IsIgnoredHeader(header string) bool { + return !v4Internal.IgnoredHeaders.IsValid(header) +} diff --git a/aws/signer/v4/v4.go b/aws/signer/v4/v4.go index 06298d4ab..6e265e292 100644 --- a/aws/signer/v4/v4.go +++ b/aws/signer/v4/v4.go @@ -443,7 +443,7 @@ func (s *httpSigner) buildCanonicalHeaders(host string, rule v4Internal.Rule, he } for k, v := range header { - if !rule.IsValid(k) { + if !s.shouldSignHeader(k, rule) { continue // ignored header } if strings.EqualFold(k, contentLengthHeader) { @@ -493,6 +493,18 @@ func (s *httpSigner) buildCanonicalHeaders(host string, rule v4Internal.Rule, he return signed, signedHeaders, canonicalHeadersStr } +func (s *httpSigner) shouldSignHeader(header string, rule v4Internal.Rule) bool { + if rule.IsValid(header) { + return true + } + if strings.EqualFold(header, authorizationHeader) { + return false + } + return slices.ContainsFunc(s.SignedHdrs, func(signedHeader string) bool { + return strings.EqualFold(signedHeader, header) + }) +} + func (s *httpSigner) buildCanonicalString(method, uri, query, signedHeaders, canonicalHeaders string) string { return strings.Join([]string{ method, diff --git a/aws/signer/v4/v4_test.go b/aws/signer/v4/v4_test.go index 8ebbd6a10..d321048d1 100644 --- a/aws/signer/v4/v4_test.go +++ b/aws/signer/v4/v4_test.go @@ -71,10 +71,9 @@ func TestPresignRequest(t *testing.T) { } expectedDate := "19700101T000000Z" - expectedHeaders := "content-length;content-type;host;x-amz-meta-other-header;x-amz-meta-other-header_with_underscore" - expectedSig := "122f0b9e091e4ba84286097e2b3404a1f1f4c4aad479adda95b7dff0ccbe5581" + expectedHeaders := "content-length;content-type;host;x-amz-meta-other-header;x-amz-meta-other-header_with_underscore;x-amz-target" + expectedSig := "266528f4c66b4b20807f199141c606c7aa81dd793592b4c6f8dc301c05691e54" expectedCred := "AKID/19700101/us-east-1/dynamodb/aws4_request" - expectedTarget := "prefix.Operation" q, err := url.ParseQuery(signed[strings.Index(signed, "?"):]) if err != nil { @@ -96,8 +95,8 @@ func TestPresignRequest(t *testing.T) { if a := q.Get("X-Amz-Meta-Other-Header"); len(a) != 0 { t.Errorf("expect %v to be empty", a) } - if e, a := expectedTarget, q.Get("X-Amz-Target"); e != a { - t.Errorf("expect %v, got %v", e, a) + if a := q.Get("X-Amz-Target"); len(a) != 0 { + t.Errorf("expect X-Amz-Target to be empty, got %v", a) } for h := range strings.SplitSeq(expectedHeaders, ";") { @@ -129,10 +128,9 @@ func TestPresignBodyWithArrayRequest(t *testing.T) { } expectedDate := "19700101T000000Z" - expectedHeaders := "content-length;content-type;host;x-amz-meta-other-header;x-amz-meta-other-header_with_underscore" - expectedSig := "e3ac55addee8711b76c6d608d762cff285fe8b627a057f8b5ec9268cf82c08b1" + expectedHeaders := "content-length;content-type;host;x-amz-meta-other-header;x-amz-meta-other-header_with_underscore;x-amz-target" + expectedSig := "f8a1f60771366686c04045b64ae1381d302c83d67d84a02567926000e3e653c4" expectedCred := "AKID/19700101/us-east-1/dynamodb/aws4_request" - expectedTarget := "prefix.Operation" if e, a := expectedSig, q.Get("X-Amz-Signature"); e != a { t.Errorf("expect %v, got %v", e, a) @@ -149,8 +147,8 @@ func TestPresignBodyWithArrayRequest(t *testing.T) { if a := q.Get("X-Amz-Meta-Other-Header"); len(a) != 0 { t.Errorf("expect %v to be empty, was not", a) } - if e, a := expectedTarget, q.Get("X-Amz-Target"); e != a { - t.Errorf("expect %v, got %v", e, a) + if a := q.Get("X-Amz-Target"); len(a) != 0 { + t.Errorf("expect X-Amz-Target to be empty, got %v", a) } for h := range strings.SplitSeq(expectedHeaders, ";") { diff --git a/s3api/utils/auth-reader.go b/s3api/utils/auth-reader.go index 3a967b976..6f456c884 100644 --- a/s3api/utils/auth-reader.go +++ b/s3api/utils/auth-reader.go @@ -59,7 +59,7 @@ func CheckValidSignature(ctx *fiber.Ctx, auth AuthData, secret, checksum string, // Create a new http request instance from fasthttp request req, err := createHttpRequestFromCtx(ctx, signedHdrs, contentLen) if err != nil { - return "", fmt.Errorf("create http request from context: %w", err) + return "", err } signer := v4.NewSigner() diff --git a/s3api/utils/presign-auth-reader.go b/s3api/utils/presign-auth-reader.go index 613ba78c1..2b438ca61 100644 --- a/s3api/utils/presign-auth-reader.go +++ b/s3api/utils/presign-auth-reader.go @@ -54,7 +54,7 @@ func CheckPresignedSignature(ctx *fiber.Ctx, auth AuthData, secret string) error // Create a new http request instance from fasthttp request req, err := createPresignedHttpRequestFromCtx(ctx, signedHdrs, contentLength) if err != nil { - return fmt.Errorf("create http request from context: %w", err) + return err } date, _ := time.Parse(iso8601Format, auth.Date) diff --git a/s3api/utils/signed_headers_test.go b/s3api/utils/signed_headers_test.go new file mode 100644 index 000000000..f40159e96 --- /dev/null +++ b/s3api/utils/signed_headers_test.go @@ -0,0 +1,227 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package utils + +import ( + "context" + "net/http" + "net/url" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/gofiber/fiber/v2" + "github.com/stretchr/testify/require" + "github.com/valyala/fasthttp" + v4 "github.com/versity/versitygw/aws/signer/v4" + "github.com/versity/versitygw/s3err" +) + +const signedHeadersTestRegion = "us-east-1" + +var signedHeadersTestCreds = aws.Credentials{ + AccessKeyID: "AKID", + SecretAccessKey: "SECRET", +} + +func TestCheckPresignedSignatureRejectsUnsignedAmzHeader(t *testing.T) { + signedURL := buildPresignedURL(t, nil) + ctx := fiberCtxFromURL(t, http.MethodPut, signedURL, http.Header{ + "X-Amz-Copy-Source": []string{"source/key"}, + }) + authData, err := ParsePresignedURIParts(ctx, signedHeadersTestRegion) + require.NoError(t, err) + + err = CheckPresignedSignature(ctx, authData, signedHeadersTestCreds.SecretAccessKey) + requireHeadersNotSigned(t, err, "x-amz-copy-source") +} + +func TestCheckPresignedSignatureAllowsSignedAmzHeader(t *testing.T) { + headers := http.Header{ + "X-Amz-Copy-Source": []string{"source/key"}, + } + signedURL := buildPresignedURL(t, headers) + ctx := fiberCtxFromURL(t, http.MethodPut, signedURL, headers) + authData, err := ParsePresignedURIParts(ctx, signedHeadersTestRegion) + require.NoError(t, err) + + err = CheckPresignedSignature(ctx, authData, signedHeadersTestCreds.SecretAccessKey) + require.NoError(t, err) +} + +func TestCheckPresignedSignatureAllowsUnsignedNonAmzHeader(t *testing.T) { + signedURL := buildPresignedURL(t, nil) + ctx := fiberCtxFromURL(t, http.MethodPut, signedURL, http.Header{ + "Content-Type": []string{"text/plain"}, + "X-Custom-Header": []string{"value"}, + }) + authData, err := ParsePresignedURIParts(ctx, signedHeadersTestRegion) + require.NoError(t, err) + + err = CheckPresignedSignature(ctx, authData, signedHeadersTestCreds.SecretAccessKey) + require.NoError(t, err) +} + +func TestCheckValidSignatureRejectsUnsignedAmzHeader(t *testing.T) { + ctx, authData, signingTime := signedHeaderAuthCtx(t, nil, http.Header{ + "X-Amz-Tagging": []string{"a=b"}, + }) + + _, err := CheckValidSignature(ctx, authData, signedHeadersTestCreds.SecretAccessKey, unsignedPayload, signingTime, 0) + requireHeadersNotSigned(t, err, "x-amz-tagging") +} + +func TestCheckValidSignatureAllowsSignedAmzHeader(t *testing.T) { + ctx, authData, signingTime := signedHeaderAuthCtx(t, http.Header{ + "X-Amz-Tagging": []string{"a=b"}, + }, nil) + + _, err := CheckValidSignature(ctx, authData, signedHeadersTestCreds.SecretAccessKey, unsignedPayload, signingTime, 0) + require.NoError(t, err) +} + +func TestCheckValidSignatureAllowsUnsignedNonAmzHeader(t *testing.T) { + ctx, authData, signingTime := signedHeaderAuthCtx(t, nil, http.Header{ + "Content-Type": []string{"text/plain"}, + "X-Custom-Header": []string{"value"}, + }) + + _, err := CheckValidSignature(ctx, authData, signedHeadersTestCreds.SecretAccessKey, unsignedPayload, signingTime, 0) + require.NoError(t, err) +} + +func TestCheckPresignedSignatureRejectsUnsignedAmzHeaderPattern(t *testing.T) { + signedURL := buildPresignedURL(t, nil) + ctx := fiberCtxFromURL(t, http.MethodPut, signedURL, http.Header{ + "X-Amz-Some-Other-Header": []string{"value"}, + }) + authData, err := ParsePresignedURIParts(ctx, signedHeadersTestRegion) + require.NoError(t, err) + + err = CheckPresignedSignature(ctx, authData, signedHeadersTestCreds.SecretAccessKey) + requireHeadersNotSigned(t, err, "x-amz-some-other-header") +} + +func TestCheckValidSignatureRejectsUnsignedAmzHeaderPattern(t *testing.T) { + ctx, authData, signingTime := signedHeaderAuthCtx(t, nil, http.Header{ + "X-Amz-Some-Other-Header": []string{"value"}, + }) + + _, err := CheckValidSignature(ctx, authData, signedHeadersTestCreds.SecretAccessKey, unsignedPayload, signingTime, 0) + requireHeadersNotSigned(t, err, "x-amz-some-other-header") +} + +func buildPresignedURL(t *testing.T, headers http.Header) string { + t.Helper() + + req, err := http.NewRequest(http.MethodPut, "http://example.com/bucket/key?X-Amz-Expires=600", nil) + require.NoError(t, err) + req.Header = headers.Clone() + if req.Header == nil { + req.Header = make(http.Header) + } + + signer := v4.NewSigner() + signedURL, _, _, err := signer.PresignHTTP( + context.Background(), + signedHeadersTestCreds, + req, + unsignedPayload, + service, + signedHeadersTestRegion, + time.Now().UTC(), + nil, + func(options *v4.SignerOptions) { + options.DisableURIPathEscaping = true + }, + ) + require.NoError(t, err) + + return signedURL +} + +func signedHeaderAuthCtx(t *testing.T, signedHeaders, extraHeaders http.Header) (*fiber.Ctx, AuthData, time.Time) { + t.Helper() + + signingTime := time.Now().UTC() + req, err := http.NewRequest(http.MethodPut, "http://example.com/bucket/key", nil) + require.NoError(t, err) + req.Header = signedHeaders.Clone() + if req.Header == nil { + req.Header = make(http.Header) + } + + signer := v4.NewSigner() + _, err = signer.SignHTTP( + context.Background(), + signedHeadersTestCreds, + req, + unsignedPayload, + service, + signedHeadersTestRegion, + signingTime, + nil, + func(options *v4.SignerOptions) { + options.DisableURIPathEscaping = true + }, + ) + require.NoError(t, err) + + headers := req.Header.Clone() + for key, values := range extraHeaders { + for _, value := range values { + headers.Add(key, value) + } + } + + ctx := fiberCtxFromURL(t, http.MethodPut, req.URL.String(), headers) + authData, err := ParseAuthorization(ctx.Get("Authorization")) + require.NoError(t, err) + + return ctx, authData, signingTime +} + +func fiberCtxFromURL(t *testing.T, method, rawURL string, headers http.Header) *fiber.Ctx { + t.Helper() + + parsedURL, err := url.Parse(rawURL) + require.NoError(t, err) + + app := fiber.New() + ctx := app.AcquireCtx(&fasthttp.RequestCtx{}) + t.Cleanup(func() { + app.ReleaseCtx(ctx) + }) + + ctx.Request().Header.SetMethod(method) + ctx.Request().SetRequestURI(parsedURL.RequestURI()) + ctx.Request().Header.SetHost(parsedURL.Host) + for key, values := range headers { + for _, value := range values { + ctx.Request().Header.Add(key, value) + } + } + + return ctx +} + +func requireHeadersNotSigned(t *testing.T, err error, expected string) { + t.Helper() + + require.Error(t, err) + serr, ok := err.(s3err.HeadersNotSignedError) + require.Truef(t, ok, "expected HeadersNotSignedError, got %T", err) + require.Equal(t, "AccessDenied", serr.Code) + require.Equal(t, expected, serr.HeadersNotSigned) +} diff --git a/s3api/utils/utils.go b/s3api/utils/utils.go index 914821968..2516a3cbb 100644 --- a/s3api/utils/utils.go +++ b/s3api/utils/utils.go @@ -25,6 +25,7 @@ import ( "net/http" "net/url" "regexp" + "slices" "strconv" "strings" "sync/atomic" @@ -33,6 +34,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/gofiber/fiber/v2" "github.com/valyala/fasthttp" + signerV4 "github.com/versity/versitygw/aws/signer/v4" "github.com/versity/versitygw/debuglogger" "github.com/versity/versitygw/s3err" "github.com/versity/versitygw/s3response" @@ -133,12 +135,8 @@ func createHttpRequestFromCtx(ctx *fiber.Ctx, signedHdrs []string, contentLength return nil, errors.New("error in creating an http request") } - // Set the request headers - for key, value := range req.Header.All() { - keyStr := string(key) - if includeHeader(keyStr, signedHdrs) { - httpReq.Header.Add(keyStr, string(value)) - } + if err := addRequestHeadersFromCtx(ctx, httpReq, signedHdrs); err != nil { + return nil, err } // make sure all headers in the signed headers are present @@ -195,12 +193,8 @@ func createPresignedHttpRequestFromCtx(ctx *fiber.Ctx, signedHdrs []string, cont if err != nil { return nil, errors.New("error in creating an http request") } - // Set the request headers - for key, value := range req.Header.All() { - keyStr := string(key) - if includeHeader(keyStr, signedHdrs) { - httpReq.Header.Add(keyStr, string(value)) - } + if err := addRequestHeadersFromCtx(ctx, httpReq, signedHdrs); err != nil { + return nil, err } // Check if Content-Length in signed headers @@ -344,12 +338,31 @@ func IsValidBucketName(bucket string) bool { } func includeHeader(hdr string, signedHdrs []string) bool { - for _, shdr := range signedHdrs { - if strings.EqualFold(hdr, shdr) { - return true + return slices.ContainsFunc(signedHdrs, func(shdr string) bool { + return strings.EqualFold(hdr, shdr) + }) +} + +func addRequestHeadersFromCtx(ctx *fiber.Ctx, httpReq *http.Request, signedHdrs []string) error { + headersNotSigned := []string{} + for key, value := range ctx.Request().Header.All() { + keyStr := string(key) + if includeHeader(keyStr, signedHdrs) || signerV4.IsIgnoredHeader(keyStr) { + httpReq.Header.Add(keyStr, string(value)) + continue + } + if signerV4.IsRequiredSignedHeader(keyStr) { + lowerKey := strings.ToLower(keyStr) + headersNotSigned = append(headersNotSigned, lowerKey) } } - return false + + if len(headersNotSigned) != 0 { + debuglogger.Logf("headers present in request but not included in SignedHeaders: %q", strings.Join(headersNotSigned, ", ")) + return s3err.GetHeadersNotSignedErr(headersNotSigned) + } + + return nil } // expiration time window diff --git a/s3err/headers-not-signed-error.go b/s3err/headers-not-signed-error.go new file mode 100644 index 000000000..b96a7f933 --- /dev/null +++ b/s3err/headers-not-signed-error.go @@ -0,0 +1,61 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package s3err + +import ( + "encoding/xml" + "net/http" + "strings" +) + +// HeadersNotSignedError is returned when signature verification receives +// required signed headers that were omitted from SignedHeaders. +type HeadersNotSignedError struct { + APIError + HeadersNotSigned string +} + +func (e HeadersNotSignedError) XMLBody(requestID, hostID string) []byte { + return encodeResponse(struct { + XMLName xml.Name `xml:"Error"` + Code string + Message string + HeadersNotSigned string `xml:",omitempty"` + RequestID string `xml:"RequestId,omitempty"` + HostID string `xml:"HostId,omitempty"` + }{ + Code: e.Code, + Message: e.Description, + HeadersNotSigned: e.HeadersNotSigned, + RequestID: requestID, + HostID: hostID, + }) +} + +func (e HeadersNotSignedError) Is(target error) bool { + t, ok := target.(APIError) + return ok && e.APIError == t +} + +func GetHeadersNotSignedErr(headers []string) HeadersNotSignedError { + return HeadersNotSignedError{ + APIError: APIError{ + Code: "AccessDenied", + Description: "There were headers present in the request which were not signed", + HTTPStatusCode: http.StatusForbidden, + }, + HeadersNotSigned: strings.Join(headers, ", "), + } +} diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 63e9dbd34..fe8b19d5c 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -37,6 +37,8 @@ func TestAuthentication(ts *TestState) { ts.Run(Authentication_incorrect_payload_hash) ts.Run(Authentication_invalid_sha256_payload_hash) ts.Run(Authentication_md5) + ts.Run(Authentication_unsigned_required_header) + ts.Run(Authentication_unsigned_non_required_header) ts.Run(Authentication_signature_error_incorrect_secret_key) ts.Run(Authentication_sigv2_not_supported) ts.Run(Authentication_with_expect_header) @@ -57,6 +59,8 @@ func TestPresignedAuthentication(ts *TestState) { ts.Run(PresignedAuth_dates_mismatch) ts.Run(PresignedAuth_non_existing_access_key_id) ts.Run(PresignedAuth_missing_signed_headers_query_param) + ts.Run(PresignedAuth_unsigned_required_header) + ts.Run(PresignedAuth_unsigned_non_required_header) ts.Run(PresignedAuth_missing_expiration_query_param) ts.Run(PresignedAuth_invalid_expiration_query_param) ts.Run(PresignedAuth_negative_expiration_query_param) @@ -1304,6 +1308,8 @@ func GetIntTests() IntTests { "Authentication_incorrect_payload_hash": Authentication_incorrect_payload_hash, "Authentication_invalid_sha256_payload_hash": Authentication_invalid_sha256_payload_hash, "Authentication_md5": Authentication_md5, + "Authentication_unsigned_required_header": Authentication_unsigned_required_header, + "Authentication_unsigned_non_required_header": Authentication_unsigned_non_required_header, "Authentication_signature_error_incorrect_secret_key": Authentication_signature_error_incorrect_secret_key, "Authentication_sigv2_not_supported": Authentication_sigv2_not_supported, "Authentication_with_expect_header": Authentication_with_expect_header, @@ -1321,6 +1327,8 @@ func GetIntTests() IntTests { "PresignedAuth_dates_mismatch": PresignedAuth_dates_mismatch, "PresignedAuth_non_existing_access_key_id": PresignedAuth_non_existing_access_key_id, "PresignedAuth_missing_signed_headers_query_param": PresignedAuth_missing_signed_headers_query_param, + "PresignedAuth_unsigned_required_header": PresignedAuth_unsigned_required_header, + "PresignedAuth_unsigned_non_required_header": PresignedAuth_unsigned_non_required_header, "PresignedAuth_missing_expiration_query_param": PresignedAuth_missing_expiration_query_param, "PresignedAuth_invalid_expiration_query_param": PresignedAuth_invalid_expiration_query_param, "PresignedAuth_negative_expiration_query_param": PresignedAuth_negative_expiration_query_param, diff --git a/tests/integration/presigned_urls.go b/tests/integration/presigned_urls.go index 933b117f8..5dc8d3fdd 100644 --- a/tests/integration/presigned_urls.go +++ b/tests/integration/presigned_urls.go @@ -459,6 +459,66 @@ func PresignedAuth_missing_signed_headers_query_param(s *S3Conf) error { }) } +func PresignedAuth_unsigned_required_header(s *S3Conf) error { + testName := "PresignedAuth_unsigned_required_header" + return presignedAuthHandler(s, testName, func(client *s3.PresignClient, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + v4req, err := client.PresignPutObject(ctx, &s3.PutObjectInput{Bucket: &bucket, Key: getPtr("my-obj")}) + cancel() + if err != nil { + return err + } + + req, err := http.NewRequest(v4req.Method, v4req.URL, nil) + if err != nil { + return err + } + + req.Header.Set("X-Amz-Copy-Source", "source-bucket/source-key") + req.Header.Set("X-Amz-Tagging", "a=b") + + resp, err := s.httpClient.Do(req) + if err != nil { + return err + } + + return checkHTTPResponseApiErr(resp, s3err.GetHeadersNotSignedErr([]string{"x-amz-copy-source", "x-amz-tagging"})) + }) +} + +func PresignedAuth_unsigned_non_required_header(s *S3Conf) error { + testName := "PresignedAuth_unsigned_non_required_header" + return presignedAuthHandler(s, testName, func(client *s3.PresignClient, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + v4req, err := client.PresignPutObject(ctx, &s3.PutObjectInput{Bucket: &bucket, Key: getPtr("my-obj")}) + cancel() + if err != nil { + return err + } + + req, err := http.NewRequest(v4req.Method, v4req.URL, nil) + if err != nil { + return err + } + + req.Header.Set("Content-Type", "text/plain") + req.Header.Set("X-Custom-Header", "value") + req.Header.Set("X-Another-Custom-Header", "value") + + resp, err := s.httpClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("expected response status code to be %v, instead got %v", http.StatusOK, resp.StatusCode) + } + + return nil + }) +} + func PresignedAuth_missing_expiration_query_param(s *S3Conf) error { testName := "PresignedAuth_missing_expiration_query_param" return presignedAuthHandler(s, testName, func(client *s3.PresignClient, bucket string) error { diff --git a/tests/integration/sigv4_auth.go b/tests/integration/sigv4_auth.go index a600753c7..b2650d68d 100644 --- a/tests/integration/sigv4_auth.go +++ b/tests/integration/sigv4_auth.go @@ -25,6 +25,7 @@ import ( "strings" "time" + "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/versity/versitygw/s3err" ) @@ -501,20 +502,7 @@ func Authentication_incorrect_payload_hash(s *S3Conf) error { func Authentication_md5(s *S3Conf) error { testName := "Authentication_md5" - bucket := getBucketName() - return authHandler(s, &authConfig{ - testName: testName, - method: http.MethodPut, - body: nil, - service: "s3", - date: time.Now(), - path: fmt.Sprintf("%s/obj", bucket), - }, func(req *http.Request) error { - err := setup(s, bucket) - if err != nil { - return err - } - + return actionHandler(s, testName, func(_ *s3.Client, bucket string) error { sum := md5.Sum(nil) emptyMd5 := base64.StdEncoding.EncodeToString(sum[:]) @@ -528,7 +516,12 @@ func Authentication_md5(s *S3Conf) error { // valid md5, but incorrect {"XrY7u+Ae7tCTyyK7j1rNww==", s3err.GetBadDigestErr(emptyMd5, base64ToHexString("XrY7u+Ae7tCTyyK7j1rNww=="))}, } { - req.Header.Set("Content-Md5", test.md5) + req, err := createSignedReq(http.MethodPut, s.endpoint, fmt.Sprintf("%s/obj", bucket), s.awsID, s.awsSecret, "s3", s.awsRegion, "", nil, time.Now(), map[string]string{ + "Content-Md5": test.md5, + }) + if err != nil { + return err + } resp, err := s.httpClient.Do(req) if err != nil { @@ -536,15 +529,56 @@ func Authentication_md5(s *S3Conf) error { } if err := checkHTTPResponseApiErr(resp, test.err); err != nil { - return fmt.Errorf("test %v failed: %v", i+1, err) + return fmt.Errorf("test %v failed: %w", i+1, err) } } - err = teardown(s, bucket) + return nil + }) +} + +func Authentication_unsigned_required_header(s *S3Conf) error { + testName := "Authentication_unsigned_required_header" + return actionHandler(s, testName, func(_ *s3.Client, bucket string) error { + req, err := createSignedReq(http.MethodPut, s.endpoint, fmt.Sprintf("%s/obj", bucket), s.awsID, s.awsSecret, "s3", s.awsRegion, "", nil, time.Now(), nil) if err != nil { return err } + req.Header.Set("X-Amz-Copy-Source", "source-bucket/source-key") + req.Header.Set("X-Amz-Tagging", "a=b") + + resp, err := s.httpClient.Do(req) + if err != nil { + return err + } + + return checkHTTPResponseApiErr(resp, s3err.GetHeadersNotSignedErr([]string{"x-amz-copy-source", "x-amz-tagging"})) + }) +} + +func Authentication_unsigned_non_required_header(s *S3Conf) error { + testName := "Authentication_unsigned_non_required_header" + return actionHandler(s, testName, func(_ *s3.Client, bucket string) error { + req, err := createSignedReq(http.MethodPut, s.endpoint, fmt.Sprintf("%s/obj", bucket), s.awsID, s.awsSecret, "s3", s.awsRegion, "", nil, time.Now(), nil) + if err != nil { + return err + } + + req.Header.Set("Content-Type", "text/plain") + req.Header.Set("X-Custom-Header", "value") + req.Header.Set("X-Another-Custom-Header", "value") + + resp, err := s.httpClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("expected response status code to be %v, instead got %v", http.StatusOK, resp.StatusCode) + } + return nil }) } diff --git a/tests/integration/utils.go b/tests/integration/utils.go index 256bc4f11..09a29b44e 100644 --- a/tests/integration/utils.go +++ b/tests/integration/utils.go @@ -426,6 +426,7 @@ type APIErrorResponse struct { StringToSignBytes string `xml:"StringToSignBytes,omitempty"` CanonicalRequest string `xml:"CanonicalRequest,omitempty"` CanonicalRequestBytes string `xml:"CanonicalRequestBytes,omitempty"` + HeadersNotSigned string `xml:"HeadersNotSigned,omitempty"` RequestID string `xml:"RequestId,omitempty"` HostID string `xml:"HostId,omitempty"` } @@ -558,6 +559,8 @@ func compareS3ApiErr(expected s3err.S3Error, received *APIErrorResponse) error { ) case s3err.MalformedAuthError: return compareErrField("Region", err.Region, received.Region) + case s3err.HeadersNotSignedError: + return compareErrField("HeadersNotSigned", err.HeadersNotSigned, received.HeadersNotSigned) case s3err.NoSuchUploadError: return compareErrField("UploadId", err.UploadId, received.UploadId) case s3err.NoSuchVersionError: