From de3fe800cb0791f2b047e26600f2c25ccaa5ab21 Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Sun, 23 Nov 2025 19:17:17 -0500 Subject: [PATCH 1/6] Add expiry validation for S3 gateway requests This change adds validation for the `X-Amz-Expires` parameter in presigned URLs. Changes: - Parse `X-Amz-Expires` and validate within allowed range - Check expiration time for presigned URLs and deny request if past expiration time. - Add an explicit check for all required parameters for presigned URLs Fixes #9599 --- pkg/gateway/sig/sig.go | 2 + pkg/gateway/sig/v4.go | 101 +++++++- .../sig/v4_expiration_internal_test.go | 242 ++++++++++++++++++ 3 files changed, 341 insertions(+), 4 deletions(-) create mode 100644 pkg/gateway/sig/v4_expiration_internal_test.go diff --git a/pkg/gateway/sig/sig.go b/pkg/gateway/sig/sig.go index 305c7cc19de..3aba7d5f309 100644 --- a/pkg/gateway/sig/sig.go +++ b/pkg/gateway/sig/sig.go @@ -99,6 +99,8 @@ func (c *chainedAuthenticator) Parse() (SigContext, error) { if err == nil { c.chosen = method return sigContext, nil + } else if !errors.Is(err, ErrHeaderMalformed) { + return nil, err } } return nil, gwErrors.ErrMissingFields diff --git a/pkg/gateway/sig/v4.go b/pkg/gateway/sig/v4.go index 88a1b67e5be..0bac3da4a3d 100644 --- a/pkg/gateway/sig/v4.go +++ b/pkg/gateway/sig/v4.go @@ -33,6 +33,14 @@ const ( v4scopeTerminator = "aws4_request" v4timeFormat = "20060102T150405Z" v4shortTimeFormat = "20060102" + maxExpires = 604800 + + v4AmzAlgorithm = "X-Amz-Algorithm" + v4AmzCredential = "X-Amz-Credential" + v4AmzSignature = "X-Amz-Signature" + v4AmzDate = "X-Amz-Date" + v4AmzSignedHeaders = "X-Amz-SignedHeaders" + v4AmzExpires = "X-Amz-Expires" ) var ( @@ -43,12 +51,14 @@ var ( type V4Auth struct { AccessKeyID string Date string + Expires int64 Region string Service string SignedHeaders []string SignedHeadersString string Signature string ChecksumAlgorithm string + IsPresigned bool } func (a V4Auth) GetAccessKeyID() string { @@ -61,6 +71,40 @@ func splitHeaders(headers string) []string { return headerValues } +func parseExpires(expiresStr string) (int64, error) { + expires, err := strconv.ParseInt(expiresStr, 10, 64) + if err != nil { + // handles both empty string and non-int string + return 0, errors.ErrMalformedExpires + } + if expires < 0 { + return 0, errors.ErrNegativeExpires + } + if expires > maxExpires { + return 0, errors.ErrMaximumExpires + } + return expires, nil +} + +func hasRequiredParams(values url.Values) bool { + var requiredPresignedParams = []string{ + v4AmzAlgorithm, + v4AmzCredential, + v4AmzSignature, + v4AmzDate, + v4AmzSignedHeaders, + v4AmzExpires, + } + + for _, param := range requiredPresignedParams { + if _, ok := values[param]; !ok { + return false + } + } + + return true +} + func ParseV4AuthContext(r *http.Request) (V4Auth, error) { var ctx V4Auth @@ -88,16 +132,22 @@ func ParseV4AuthContext(r *http.Request) (V4Auth, error) { signatureHeaders := result["SignatureHeaders"] ctx.SignedHeaders = splitHeaders(signatureHeaders) ctx.SignedHeadersString = signatureHeaders + ctx.IsPresigned = false return ctx, nil } - // otherwise, see if we have all the required query parameters query := r.URL.Query() - algorithm := query.Get("X-Amz-Algorithm") + // otherwise, see if we have all the required query parameters + if !hasRequiredParams(query) { + return ctx, errors.ErrInvalidQueryParams + } + ctx.IsPresigned = true + + algorithm := query.Get(v4AmzAlgorithm) if len(algorithm) == 0 || !strings.EqualFold(algorithm, V4authHeaderPrefix) { return ctx, errors.ErrInvalidQuerySignatureAlgo } - credentialScope := query.Get("X-Amz-Credential") + credentialScope := query.Get(v4AmzCredential) if len(credentialScope) == 0 { return ctx, errors.ErrMissingCredTag } @@ -116,7 +166,13 @@ func ParseV4AuthContext(r *http.Request) (V4Auth, error) { ctx.Region = credsResult["Region"] ctx.Service = credsResult["Service"] - ctx.SignedHeadersString = query.Get("X-Amz-SignedHeaders") + expires, err := parseExpires(query.Get(v4AmzExpires)) + if err != nil { + return ctx, err + } + ctx.Expires = expires + + ctx.SignedHeadersString = query.Get(v4AmzSignedHeaders) headers := splitHeaders(ctx.SignedHeadersString) ctx.SignedHeaders = headers ctx.Signature = query.Get(v4SignatureHeader) @@ -144,6 +200,11 @@ func V4Verify(auth V4Auth, credentials *model.Credential, r *http.Request) error return errors.ErrSignatureDoesNotMatch } + // check that the request hasn't expired + if err := ctx.verifyExpiration(); err != nil { + return err + } + // wrap body with verifier reader, err := ctx.reader(r.Body, credentials) if err != nil { @@ -293,6 +354,38 @@ func (ctx *verificationCtx) getAmzDate() (string, error) { return amzDate, nil } +func (ctx *verificationCtx) verifyExpiration() error { + if !ctx.AuthValue.IsPresigned { + // TODO: we currently don't have handling for this + return nil + } + // Get and validate the request timestamp + amzDate, err := ctx.getAmzDate() + if err != nil { + return err + } + + requestTime, err := time.Parse(v4timeFormat, amzDate) + if err != nil { + return err + } + + now := time.Now().UTC() + timeDiff := now.Sub(requestTime) + + // Check for requests from the future and allow small clock skew + if timeDiff < 0 && timeDiff.Abs() > 5*time.Minute { + return errors.ErrRequestNotReadyYet + } + + expirationTime := requestTime.Add(time.Duration(ctx.AuthValue.Expires) * time.Second) + if now.After(expirationTime) { + return errors.ErrExpiredPresignRequest + } + + return nil +} + func sign(key []byte, msg string) []byte { h := hmac.New(sha256.New, key) _, _ = h.Write([]byte(msg)) diff --git a/pkg/gateway/sig/v4_expiration_internal_test.go b/pkg/gateway/sig/v4_expiration_internal_test.go new file mode 100644 index 00000000000..39b60c43e32 --- /dev/null +++ b/pkg/gateway/sig/v4_expiration_internal_test.go @@ -0,0 +1,242 @@ +package sig + +import ( + "net/http" + "net/url" + "testing" + "time" + + "github.com/treeverse/lakefs/pkg/gateway/errors" +) + +func TestVerifyExpiration(t *testing.T) { + now := time.Now().UTC() + + testCases := []struct { + name string + isPresigned bool + expires int64 + requestTime time.Time + expectedError error + }{ + // Presigned URL tests + { + name: "valid presigned URL", + isPresigned: true, + expires: 3600, + requestTime: now, + expectedError: nil, + }, + { + name: "expired presigned URL", + isPresigned: true, + expires: 10, + requestTime: now.Add(-1 * time.Minute), + expectedError: errors.ErrExpiredPresignRequest, + }, + { + name: "presigned URL with zero expiration", + isPresigned: true, + expires: 0, + requestTime: now.Add(-1 * time.Second), + expectedError: errors.ErrExpiredPresignRequest, + }, + { + name: "presigned URL at max expiration (7 days)", + isPresigned: true, + expires: 604800, + requestTime: now, + expectedError: nil, + }, + { + name: "presigned URL future request beyond clock skew", + isPresigned: true, + expires: 3600, + requestTime: now.Add(10 * time.Minute), + expectedError: errors.ErrRequestNotReadyYet, + }, + { + name: "presigned URL future request within clock skew", + isPresigned: true, + expires: 3600, + requestTime: now.Add(3 * time.Minute), + expectedError: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create a minimal verification context + amzDate := tc.requestTime.Format(v4timeFormat) + // Use the same date for the credential scope (must match calendar day) + dateStamp := tc.requestTime.Format(v4shortTimeFormat) + + req, _ := http.NewRequest(http.MethodGet, "https://example.com/test", nil) + + // Set X-Amz-Date in query for presigned URLs + if tc.isPresigned { + query := url.Values{} + query.Set("X-Amz-Date", amzDate) + req.URL.RawQuery = query.Encode() + } + + ctx := &verificationCtx{ + Request: req, + Query: req.URL.Query(), + AuthValue: V4Auth{ + Date: dateStamp, + IsPresigned: tc.isPresigned, + Expires: tc.expires, + }, + } + + err := ctx.verifyExpiration() + + if tc.expectedError == nil { + if err != nil { + t.Errorf("expected no error, got %v", err) + } + } else { + if err != tc.expectedError { + t.Errorf("expected error %v, got %v", tc.expectedError, err) + } + } + }) + } +} + +func TestParseExpires(t *testing.T) { + testCases := []struct { + name string + expiresStr string + expectedValue int64 + expectedError error + }{ + { + name: "valid expires", + expiresStr: "3600", + expectedValue: 3600, + expectedError: nil, + }, + { + name: "zero expires", + expiresStr: "0", + expectedValue: 0, + expectedError: nil, + }, + { + name: "max expires (7 days)", + expiresStr: "604800", + expectedValue: 604800, + expectedError: nil, + }, + { + name: "negative expires", + expiresStr: "-100", + expectedValue: 0, + expectedError: errors.ErrNegativeExpires, + }, + { + name: "expires over 7 days", + expiresStr: "604801", + expectedValue: 0, + expectedError: errors.ErrMaximumExpires, + }, + { + name: "malformed expires - not a number", + expiresStr: "abc", + expectedValue: 0, + expectedError: errors.ErrMalformedExpires, + }, + { + name: "malformed expires - decimal", + expiresStr: "3600.5", + expectedValue: 0, + expectedError: errors.ErrMalformedExpires, + }, + { + name: "malformed expires - empty string", + expiresStr: "", + expectedValue: 0, + expectedError: errors.ErrMalformedExpires, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + value, err := parseExpires(tc.expiresStr) + + if value != tc.expectedValue { + t.Errorf("expected value %d, got %d", tc.expectedValue, value) + } + + if tc.expectedError == nil { + if err != nil { + t.Errorf("expected no error, got %v", err) + } + } else { + if err != tc.expectedError { + t.Errorf("expected error %v, got %v", tc.expectedError, err) + } + } + }) + } +} + +func TestHasRequiredParams(t *testing.T) { + testCases := []struct { + name string + query url.Values + expected bool + }{ + { + name: "all required params present", + query: url.Values{ + v4AmzAlgorithm: {"AWS4-HMAC-SHA256"}, + v4AmzCredential: {"AKIAIOSFODNN7EXAMPLE/20230101/us-east-1/s3/aws4_request"}, + v4AmzSignature: {"abcd1234"}, + v4AmzDate: {"20230101T120000Z"}, + v4AmzSignedHeaders: {"host"}, + v4AmzExpires: {"3600"}, + }, + expected: true, + }, + { + name: "missing a parameter", + query: url.Values{ + v4AmzCredential: {"AKIAIOSFODNN7EXAMPLE/20230101/us-east-1/s3/aws4_request"}, + v4AmzSignature: {"abcd1234"}, + v4AmzDate: {"20230101T120000Z"}, + v4AmzSignedHeaders: {"host"}, + v4AmzExpires: {"3600"}, + }, + expected: false, + }, + { + name: "parameter is present but empty", + query: url.Values{ + v4AmzAlgorithm: {"AWS4-HMAC-SHA256"}, + v4AmzCredential: {"AKIAIOSFODNN7EXAMPLE/20230101/us-east-1/s3/aws4_request"}, + v4AmzSignature: {"abcd1234"}, + v4AmzDate: {"20230101T120000Z"}, + v4AmzSignedHeaders: {"host"}, + v4AmzExpires: {""}, + }, + expected: true, // Key exists in map, even if empty + }, + { + name: "empty query params", + query: url.Values{}, + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := hasRequiredParams(tc.query) + if result != tc.expected { + t.Errorf("expected %v, got %v", tc.expected, result) + } + }) + } +} From 45143f84333d146d0c30bb5926aa8c1727ecf838 Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Mon, 24 Nov 2025 14:06:42 -0500 Subject: [PATCH 2/6] Exclude credential const from linting --- pkg/gateway/sig/v4.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/gateway/sig/v4.go b/pkg/gateway/sig/v4.go index 0bac3da4a3d..fed293f923d 100644 --- a/pkg/gateway/sig/v4.go +++ b/pkg/gateway/sig/v4.go @@ -35,7 +35,8 @@ const ( v4shortTimeFormat = "20060102" maxExpires = 604800 - v4AmzAlgorithm = "X-Amz-Algorithm" + v4AmzAlgorithm = "X-Amz-Algorithm" + //nolint:gosec v4AmzCredential = "X-Amz-Credential" v4AmzSignature = "X-Amz-Signature" v4AmzDate = "X-Amz-Date" From 1f3d9f79ec00e6d8aa3dd1911e9f84abeedd781d Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Thu, 27 Nov 2025 17:05:51 -0500 Subject: [PATCH 3/6] Fix v4 auth to allow fallback to other auth methods This change ensures that when a request doesn't have the v4 algorithm URL parameter (`x-amz-algorithm`), the chained authenticator tries the next auth method instead of failing. Previously, requests using other signature versions would fail because v4 parsing returned and error that blocked the chain. --- pkg/gateway/sig/sig.go | 8 ++- pkg/gateway/sig/v4.go | 42 +++++++------- .../sig/v4_expiration_internal_test.go | 56 +++++++++++++------ 3 files changed, 68 insertions(+), 38 deletions(-) diff --git a/pkg/gateway/sig/sig.go b/pkg/gateway/sig/sig.go index 3aba7d5f309..15a813c9bc9 100644 --- a/pkg/gateway/sig/sig.go +++ b/pkg/gateway/sig/sig.go @@ -15,7 +15,8 @@ import ( ) var ( - ErrHeaderMalformed = errors.New("header malformed") + ErrHeaderMalformed = errors.New("header malformed") + ErrBadAuthorizationFormat = errors.New("authorization format not supported by this authenticator") // reservedObjectNames - if object matches reserved string, no need to encode them reservedObjectNames = regexp.MustCompile("^[a-zA-Z0-9-_.~/]+$") @@ -99,7 +100,10 @@ func (c *chainedAuthenticator) Parse() (SigContext, error) { if err == nil { c.chosen = method return sigContext, nil - } else if !errors.Is(err, ErrHeaderMalformed) { + } else if !errors.Is(err, ErrHeaderMalformed) && !errors.Is(err, ErrBadAuthorizationFormat) { + // ErrHeaderMalformed and ErrBadAuthorizationFormat indicate "wrong auth format, try next method". + // All other errors mean the request matched this method's format but failed validation, + // so return immediately without trying remaining methods. return nil, err } } diff --git a/pkg/gateway/sig/v4.go b/pkg/gateway/sig/v4.go index fed293f923d..8ae30f995e1 100644 --- a/pkg/gateway/sig/v4.go +++ b/pkg/gateway/sig/v4.go @@ -33,7 +33,7 @@ const ( v4scopeTerminator = "aws4_request" v4timeFormat = "20060102T150405Z" v4shortTimeFormat = "20060102" - maxExpires = 604800 + AmzPresignMaxExpires = 7 * 24 * time.Hour // 7 days or 604800 seconds v4AmzAlgorithm = "X-Amz-Algorithm" //nolint:gosec @@ -81,29 +81,36 @@ func parseExpires(expiresStr string) (int64, error) { if expires < 0 { return 0, errors.ErrNegativeExpires } - if expires > maxExpires { + if expires > int64(AmzPresignMaxExpires.Seconds()) { return 0, errors.ErrMaximumExpires } return expires, nil } -func hasRequiredParams(values url.Values) bool { - var requiredPresignedParams = []string{ - v4AmzAlgorithm, +func isV4PresignedRequest(query url.Values) error { + algorithm, ok := query[v4AmzAlgorithm] + if !ok { + // Not a V4 presigned request, try next auth method + return ErrBadAuthorizationFormat + } + if len(algorithm) == 0 || !strings.EqualFold(algorithm[0], V4authHeaderPrefix) { + return errors.ErrInvalidQuerySignatureAlgo + } + + requiredParams := []string{ v4AmzCredential, v4AmzSignature, v4AmzDate, v4AmzSignedHeaders, v4AmzExpires, } - - for _, param := range requiredPresignedParams { - if _, ok := values[param]; !ok { - return false + for _, param := range requiredParams { + if _, ok := query[param]; !ok { + return errors.ErrMissingFields } } - return true + return nil } func ParseV4AuthContext(r *http.Request) (V4Auth, error) { @@ -138,16 +145,13 @@ func ParseV4AuthContext(r *http.Request) (V4Auth, error) { } query := r.URL.Query() - // otherwise, see if we have all the required query parameters - if !hasRequiredParams(query) { - return ctx, errors.ErrInvalidQueryParams - } - ctx.IsPresigned = true - algorithm := query.Get(v4AmzAlgorithm) - if len(algorithm) == 0 || !strings.EqualFold(algorithm, V4authHeaderPrefix) { - return ctx, errors.ErrInvalidQuerySignatureAlgo + // If we couldn't find the auth header, try to parse the request as a presigned URL + if err := isV4PresignedRequest(query); err != nil { + return ctx, err } + + ctx.IsPresigned = true credentialScope := query.Get(v4AmzCredential) if len(credentialScope) == 0 { return ctx, errors.ErrMissingCredTag @@ -357,9 +361,9 @@ func (ctx *verificationCtx) getAmzDate() (string, error) { func (ctx *verificationCtx) verifyExpiration() error { if !ctx.AuthValue.IsPresigned { - // TODO: we currently don't have handling for this return nil } + // Get and validate the request timestamp amzDate, err := ctx.getAmzDate() if err != nil { diff --git a/pkg/gateway/sig/v4_expiration_internal_test.go b/pkg/gateway/sig/v4_expiration_internal_test.go index 39b60c43e32..1c4c9ca99f8 100644 --- a/pkg/gateway/sig/v4_expiration_internal_test.go +++ b/pkg/gateway/sig/v4_expiration_internal_test.go @@ -183,14 +183,14 @@ func TestParseExpires(t *testing.T) { } } -func TestHasRequiredParams(t *testing.T) { +func TestIsV4PresignedRequest(t *testing.T) { testCases := []struct { - name string - query url.Values - expected bool + name string + query url.Values + expectedError error }{ { - name: "all required params present", + name: "valid V4 presigned request", query: url.Values{ v4AmzAlgorithm: {"AWS4-HMAC-SHA256"}, v4AmzCredential: {"AKIAIOSFODNN7EXAMPLE/20230101/us-east-1/s3/aws4_request"}, @@ -199,10 +199,10 @@ func TestHasRequiredParams(t *testing.T) { v4AmzSignedHeaders: {"host"}, v4AmzExpires: {"3600"}, }, - expected: true, + expectedError: nil, }, { - name: "missing a parameter", + name: "missing algorithm - not a V4 request", query: url.Values{ v4AmzCredential: {"AKIAIOSFODNN7EXAMPLE/20230101/us-east-1/s3/aws4_request"}, v4AmzSignature: {"abcd1234"}, @@ -210,32 +210,54 @@ func TestHasRequiredParams(t *testing.T) { v4AmzSignedHeaders: {"host"}, v4AmzExpires: {"3600"}, }, - expected: false, + expectedError: ErrBadAuthorizationFormat, + }, + { + name: "invalid algorithm", + query: url.Values{ + v4AmzAlgorithm: {"AWS4-HMAC-SHA512"}, + v4AmzCredential: {"AKIAIOSFODNN7EXAMPLE/20230101/us-east-1/s3/aws4_request"}, + v4AmzSignature: {"abcd1234"}, + v4AmzDate: {"20230101T120000Z"}, + v4AmzSignedHeaders: {"host"}, + v4AmzExpires: {"3600"}, + }, + expectedError: errors.ErrInvalidQuerySignatureAlgo, + }, + { + name: "missing required param (credential)", + query: url.Values{ + v4AmzAlgorithm: {"AWS4-HMAC-SHA256"}, + v4AmzSignature: {"abcd1234"}, + v4AmzDate: {"20230101T120000Z"}, + v4AmzSignedHeaders: {"host"}, + v4AmzExpires: {"3600"}, + }, + expectedError: errors.ErrMissingFields, }, { - name: "parameter is present but empty", + name: "missing required param (expires)", query: url.Values{ v4AmzAlgorithm: {"AWS4-HMAC-SHA256"}, v4AmzCredential: {"AKIAIOSFODNN7EXAMPLE/20230101/us-east-1/s3/aws4_request"}, v4AmzSignature: {"abcd1234"}, v4AmzDate: {"20230101T120000Z"}, v4AmzSignedHeaders: {"host"}, - v4AmzExpires: {""}, }, - expected: true, // Key exists in map, even if empty + expectedError: errors.ErrMissingFields, }, { - name: "empty query params", - query: url.Values{}, - expected: false, + name: "empty query params - not a V4 request", + query: url.Values{}, + expectedError: ErrBadAuthorizationFormat, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - result := hasRequiredParams(tc.query) - if result != tc.expected { - t.Errorf("expected %v, got %v", tc.expected, result) + err := isV4PresignedRequest(tc.query) + if err != tc.expectedError { + t.Errorf("expected error %v, got %v", tc.expectedError, err) } }) } From 99a1f85f4d1e6e85c52acee6dbc1d24844a4c421 Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Mon, 1 Dec 2025 10:43:58 -0500 Subject: [PATCH 4/6] Match S3 clock skew tolerance Updates future timestamp validation to match AWS S3 behaviour. AWS tolerates future clock skew of up to 15 minutes for presigned requests. --- pkg/gateway/sig/v4.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/gateway/sig/v4.go b/pkg/gateway/sig/v4.go index 8ae30f995e1..3435bb87989 100644 --- a/pkg/gateway/sig/v4.go +++ b/pkg/gateway/sig/v4.go @@ -33,7 +33,9 @@ const ( v4scopeTerminator = "aws4_request" v4timeFormat = "20060102T150405Z" v4shortTimeFormat = "20060102" - AmzPresignMaxExpires = 7 * 24 * time.Hour // 7 days or 604800 seconds + + AmzPresignMaxExpires = 7 * 24 * time.Hour // 7 days or 604800 seconds + AmzMaxClockSkew = 15 * time.Minute // Maximum allowed clock skew (15 minutes for AWS S3 compatibility) v4AmzAlgorithm = "X-Amz-Algorithm" //nolint:gosec @@ -378,11 +380,12 @@ func (ctx *verificationCtx) verifyExpiration() error { now := time.Now().UTC() timeDiff := now.Sub(requestTime) - // Check for requests from the future and allow small clock skew - if timeDiff < 0 && timeDiff.Abs() > 5*time.Minute { + // Check for requests signed more than 15 minutes in the future (matches S3 behavior) + if timeDiff < 0 && timeDiff.Abs() > AmzMaxClockSkew { return errors.ErrRequestNotReadyYet } + // Calculate expiration from the signed time, not current time expirationTime := requestTime.Add(time.Duration(ctx.AuthValue.Expires) * time.Second) if now.After(expirationTime) { return errors.ErrExpiredPresignRequest From fdc783254093a2f14917e99a4c688280fc59042a Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Mon, 1 Dec 2025 11:11:59 -0500 Subject: [PATCH 5/6] Update test to use new constant --- pkg/gateway/sig/v4.go | 2 +- pkg/gateway/sig/v4_expiration_internal_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/gateway/sig/v4.go b/pkg/gateway/sig/v4.go index 3435bb87989..937530c06b2 100644 --- a/pkg/gateway/sig/v4.go +++ b/pkg/gateway/sig/v4.go @@ -34,7 +34,7 @@ const ( v4timeFormat = "20060102T150405Z" v4shortTimeFormat = "20060102" - AmzPresignMaxExpires = 7 * 24 * time.Hour // 7 days or 604800 seconds + AmzPresignMaxExpires = 7 * 24 * time.Hour // Maximum expiry duration for presigned URLs (7 days or 604800 seconds) AmzMaxClockSkew = 15 * time.Minute // Maximum allowed clock skew (15 minutes for AWS S3 compatibility) v4AmzAlgorithm = "X-Amz-Algorithm" diff --git a/pkg/gateway/sig/v4_expiration_internal_test.go b/pkg/gateway/sig/v4_expiration_internal_test.go index 1c4c9ca99f8..6ebd7d982b0 100644 --- a/pkg/gateway/sig/v4_expiration_internal_test.go +++ b/pkg/gateway/sig/v4_expiration_internal_test.go @@ -52,7 +52,7 @@ func TestVerifyExpiration(t *testing.T) { name: "presigned URL future request beyond clock skew", isPresigned: true, expires: 3600, - requestTime: now.Add(10 * time.Minute), + requestTime: now.Add(AmzMaxClockSkew + 1*time.Minute), expectedError: errors.ErrRequestNotReadyYet, }, { From 4c53ef94fbb8f0233cc831a7226d2b32cdfda73e Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Mon, 1 Dec 2025 18:17:01 -0500 Subject: [PATCH 6/6] Add esti test for presigned URL expiration --- esti/presign_expiry_test.go | 218 ++++++++++++++++++++++++++++++++++++ 1 file changed, 218 insertions(+) create mode 100644 esti/presign_expiry_test.go diff --git a/esti/presign_expiry_test.go b/esti/presign_expiry_test.go new file mode 100644 index 00000000000..114c49abd94 --- /dev/null +++ b/esti/presign_expiry_test.go @@ -0,0 +1,218 @@ +package esti + +import ( + "context" + "fmt" + "io" + "net/http" + "net/url" + "strconv" + "strings" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4" + "github.com/minio/minio-go/v7" + "github.com/minio/minio-go/v7/pkg/credentials" + "github.com/spf13/viper" + "github.com/stretchr/testify/require" +) + +const ( + // testS3Region is the AWS region used for S3 signature testing. + // This must match the region used in testutil.SetupTestS3Client. + testS3Region = "us-east-1" +) + +// TestS3PresignedURLExpirationWithCustomTime tests presigned URL expiration validation +// using the v4.Signer.PresignHTTP method which allows controlling the signing time +func TestS3PresignedURLExpirationWithCustomTime(t *testing.T) { + ctx, _, repo := setupTest(t) + defer tearDownTest(repo) + + blockStoreType := viper.GetString(ViperBlockstoreType) + if blockStoreType != "s3" { + t.Skip("Skipping test - only S3 blockstore type is supported for presigned URL expiration validation") + } + + // Upload a test file to use for presigned URL tests + minioClient := newMinioClient(t, credentials.NewStaticV4) + testContent := "test content for presigned URL expiration" + testPath := "main/presign-expiry-test" + + _, err := minioClient.PutObject(ctx, repo, testPath, strings.NewReader(testContent), int64(len(testContent)), minio.PutObjectOptions{}) + require.NoError(t, err, "failed to upload test file") + + accessKeyID := viper.GetString("access_key_id") + secretAccessKey := viper.GetString("secret_access_key") + s3Endpoint := viper.GetString("s3_endpoint") + + // Create v4 signer + signer := v4.NewSigner() + creds := aws.Credentials{ + AccessKeyID: accessKeyID, + SecretAccessKey: secretAccessKey, + } + + // Helper function to create presigned URL with custom signing time + createPresignedURL := func(signingTime time.Time, expiryDuration time.Duration) (string, error) { + // Build the request URL properly using url.URL + endpoint := s3Endpoint + if !strings.HasPrefix(endpoint, "http") { + if viper.GetBool("s3_endpoint_secure") { + endpoint = "https://" + endpoint + } else { + endpoint = "http://" + endpoint + } + } + + baseURL, err := url.Parse(endpoint) + if err != nil { + return "", fmt.Errorf("failed to parse endpoint: %w", err) + } + + // Construct the path: /bucket/key + baseURL.Path = fmt.Sprintf("/%s/%s", repo, testPath) + + // Create HTTP request + req, err := http.NewRequest("GET", baseURL.String(), nil) + if err != nil { + return "", err + } + + // Add required query parameters for presigning + q := req.URL.Query() + q.Set("X-Amz-Expires", strconv.FormatInt(int64(expiryDuration.Seconds()), 10)) + req.URL.RawQuery = q.Encode() + + // Calculate payload hash (UNSIGNED-PAYLOAD for presigned URLs) + payloadHash := "UNSIGNED-PAYLOAD" + + // Presign the request with custom signing time + signedURI, _, err := signer.PresignHTTP( + context.Background(), + creds, + req, + payloadHash, + "s3", + testS3Region, + signingTime, + func(opts *v4.SignerOptions) { + opts.DisableURIPathEscaping = true // S3 doesn't need additional escaping + }, + ) + if err != nil { + return "", err + } + + return signedURI, nil + } + + t.Run("valid_presigned_url", func(t *testing.T) { + // Create a presigned URL signed now with 1 hour expiry + presignedURL, err := createPresignedURL(time.Now(), time.Hour) + require.NoError(t, err, "failed to create presigned URL") + + // The presigned URL should work + resp, err := http.Get(presignedURL) + require.NoError(t, err, "failed to GET presigned URL") + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode, "expected successful response from valid presigned URL") + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err, "failed to read response body") + require.Equal(t, testContent, string(body), "content mismatch") + }) + + t.Run("expired_presigned_url", func(t *testing.T) { + // Create a presigned URL signed 15 minutes ago with 10 minute expiry + // This means it expired 5 minutes ago + signTime := time.Now().Add(-15 * time.Minute) + presignedURL, err := createPresignedURL(signTime, 10*time.Minute) + require.NoError(t, err, "failed to create presigned URL") + + // The presigned URL should be expired + resp, err := http.Get(presignedURL) + require.NoError(t, err, "failed to GET presigned URL") + defer resp.Body.Close() + + require.Equal(t, http.StatusForbidden, resp.StatusCode, "expected 403 Forbidden from expired presigned URL") + }) + + t.Run("presigned_url_within_clock_skew_tolerance", func(t *testing.T) { + // Create a presigned URL signed 10 minutes in the future (within 15 minute tolerance) + signTime := time.Now().Add(10 * time.Minute) + presignedURL, err := createPresignedURL(signTime, time.Hour) + require.NoError(t, err, "failed to create presigned URL") + + // The presigned URL should work despite being signed in the future + resp, err := http.Get(presignedURL) + require.NoError(t, err, "failed to GET presigned URL") + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode, + "expected successful response from URL signed within 15min clock skew tolerance") + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err, "failed to read response body") + require.Equal(t, testContent, string(body), "content mismatch") + }) + + t.Run("presigned_url_signed_too_far_in_future", func(t *testing.T) { + // Create a presigned URL signed 20 minutes in the future (beyond 15 minute tolerance) + signTime := time.Now().Add(20 * time.Minute) + presignedURL, err := createPresignedURL(signTime, time.Hour) + require.NoError(t, err, "failed to create presigned URL") + + // The presigned URL should be rejected + resp, err := http.Get(presignedURL) + require.NoError(t, err, "failed to GET presigned URL") + defer resp.Body.Close() + + require.Equal(t, http.StatusForbidden, resp.StatusCode, + "expected 403 Forbidden from URL signed more than 15min in the future") + }) + + t.Run("expiry_calculated_from_signed_time_not_current_time", func(t *testing.T) { + // Test that expiry is calculated from signed time, not current time + // This proves the expiration countdown starts when the URL is signed, + // not when it's accessed. + const ( + timeInPast = 2 * time.Second // How long ago the URL was "signed" + expiryDuration = 4 * time.Second // Total expiry time from signing + sleepDuration = 3 * time.Second // How long to wait before second request + ) + // Expected behavior: + // - First request: signed 2s ago + 4s expiry = 2s remaining → should work + // - After 3s sleep: signed 5s ago + 4s expiry = expired 1s ago → should fail + + signTime := time.Now().Add(-timeInPast) + presignedURL, err := createPresignedURL(signTime, expiryDuration) + require.NoError(t, err, "failed to create presigned URL") + + // First request: URL should still be valid (2 seconds remaining) + resp, err := http.Get(presignedURL) + require.NoError(t, err, "failed to GET presigned URL") + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode, + "expected successful response - URL should not be expired yet") + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err, "failed to read response body") + require.Equal(t, testContent, string(body), "content mismatch") + + // Wait for the URL to expire + time.Sleep(sleepDuration) + + // Second request: URL should now be expired (1 second past expiration) + resp2, err := http.Get(presignedURL) + require.NoError(t, err, "failed to GET presigned URL") + defer resp2.Body.Close() + + require.Equal(t, http.StatusForbidden, resp2.StatusCode, + "expected 403 Forbidden - URL should be expired after waiting") + }) +}