Add expiry validation for S3 gateway requests #9710

zubron · 2025-11-24T00:26:15Z

Closes #9599

Change Description

Bug Fix

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 if request is past expiry time.
Add an explicit check for all required parameters for presigned URLs

Testing Details

Unit tests for the verification logic were added.

Also tested using the python test in this gist

Breaking Change?

This could be considered a breaking change if users were unaware of this issue and have been relying on presigned URLs not expiring. However, this is a security issue and should be fixed.

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

N-o-Z

Looks good!

Added comment inline.
In addition:

I think it will be nice to have an esti test that checks the e2e flow with an expired presigned URL request (I think this can be done with a very short expiry) but if you think it's too much work for too less of a value let me know
These changes fix the behavior for the V4 signer but we also have the V2 which although deprecated is still being used in some cases and is still supported in our code. We might want to consult with product regarding that. If we do need to fix this for V2 let's open a separate issue for that so this PR can converge.

N-o-Z · 2025-11-28T15:07:13Z

pkg/gateway/sig/sig.go

 		if err == nil {
 			c.chosen = method
 			return sigContext, nil
+		} else if !errors.Is(err, ErrHeaderMalformed) && !errors.Is(err, ErrBadAuthorizationFormat) {


Can you please add a comment here explaining why we are singling out these error types

I've added a comment here, hopefully it explains it clearly!

pkg/gateway/sig/v4.go

N-o-Z · 2025-11-28T15:47:04Z

pkg/gateway/sig/v4.go

+	return expires, nil
+}
+
+func isV4PresignedRequest(query url.Values) error {


Nice taking this out to a separate method

N-o-Z · 2025-11-28T15:48:41Z

pkg/gateway/sig/v4.go

 		return ctx, nil
 	}

-	// otherwise, see if we have all the required query parameters


I think it's important to keep the comment.
I'd modify it to: "Otherwise try to parse request as presigned URL

Comment replaced and updated

N-o-Z · 2025-11-28T15:52:16Z

pkg/gateway/sig/v4.go


+func (ctx *verificationCtx) verifyExpiration() error {
+	if !ctx.AuthValue.IsPresigned {
+		// TODO: we currently don't have handling for this


Why is that a TODO?
Is there an expiry mechanism for regular requests?

N-o-Z · 2025-11-28T17:11:18Z

pkg/gateway/sig/v4.go

+	timeDiff := now.Sub(requestTime)
+
+	// Check for requests from the future and allow small clock skew
+	if timeDiff < 0 && timeDiff.Abs() > 5*time.Minute {


Why are we checking for 5min?
What's the behavior in AWS for presigned requests that are less than 5 min in the future?
Please document

I had a misunderstanding here of how AWS handles clock skew for signed requests. After testing against AWS S3: AWS validates clock skew for presigned URLs when X-Amz-Date is in the future. If that time is >15 minutes ahead of the server time, the requests are rejected and anything within 15 minutes is tolerated. No clock skew check for past timestamps (only expiration). I'll fix this and add comments.

N-o-Z · 2025-11-28T17:13:01Z

pkg/gateway/sig/v4_expiration_internal_test.go

+	"github.com/treeverse/lakefs/pkg/gateway/errors"
+)
+
+func TestVerifyExpiration(t *testing.T) {


Nice tests - do we want to also add tests for non-presigned with expiry and check our behavior is consistent with S3?

I'm going to add those tests in a follow up.

N-o-Z · 2025-11-28T17:14:33Z

pkg/gateway/sig/v4_expiration_internal_test.go

+	}
+}
+
+func TestParseExpires(t *testing.T) {


We should test for nil expires as well (no header)

parseExpires is only called after isV4PresignedRequest validates that X-Amz-Expires exists in the request. At that point, we have a string value which can't be nil. The empty string case is already covered in this test and the tests for isV4PresignedRequest check for the missing expiry param. Did you have a different test in mind?

parseExpires is only called after isV4PresignedRequest validates that X-Amz-Expires exists in the request. At that point, query.Get() returns a string (which can't be nil). The empty string case is already covered by the existing test and the tests for isV4PresignedRequest check for the missing expiry param. Did you have a different test in mind?

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.

Updates future timestamp validation to match AWS S3 behaviour. AWS tolerates future clock skew of up to 15 minutes for presigned requests.

zubron · 2025-12-02T16:36:40Z

Thanks for the review @N-o-Z! I think I've addressed your comments, and I also added some esti tests to cover the different expiry situations. I agree that anything related to V2 can be addressed in a follow up 👍

N-o-Z

Thank you for addressing the comments and adding the tests!

N-o-Z · 2025-12-02T17:02:00Z

pkg/gateway/sig/v4.go

 		return errors.ErrRequestNotReadyYet
 	}

+	// Calculate expiration from the signed time, not current time


N-o-Z · 2025-12-02T17:05:44Z

Thank you for addressing the comments and adding the tests!

Please link follow-up issue to this task

zubron force-pushed the fix/presigned-urls-ignore-expiration-9599 branch from 01442fe to 3915765 Compare November 24, 2025 15:16

zubron added the include-changelog PR description should be included in next release changelog label Nov 24, 2025

zubron force-pushed the fix/presigned-urls-ignore-expiration-9599 branch 3 times, most recently from 77a8138 to f5fb7ba Compare November 27, 2025 15:00

zubron added 2 commits November 27, 2025 17:09

Exclude credential const from linting

45143f8

zubron force-pushed the fix/presigned-urls-ignore-expiration-9599 branch 2 times, most recently from 375ec38 to 07ff93a Compare November 27, 2025 22:14

zubron marked this pull request as ready for review November 27, 2025 23:21

zubron requested a review from a team November 27, 2025 23:23

N-o-Z requested changes Nov 28, 2025

View reviewed changes

zubron force-pushed the fix/presigned-urls-ignore-expiration-9599 branch from 07ff93a to d5ee853 Compare December 1, 2025 14:25

zubron added 2 commits December 1, 2025 09:47

Match S3 clock skew tolerance

99a1f85

Updates future timestamp validation to match AWS S3 behaviour. AWS tolerates future clock skew of up to 15 minutes for presigned requests.

zubron force-pushed the fix/presigned-urls-ignore-expiration-9599 branch from d5ee853 to 99a1f85 Compare December 1, 2025 15:49

zubron added 2 commits December 1, 2025 11:11

Update test to use new constant

fdc7832

Add esti test for presigned URL expiration

4c53ef9

N-o-Z approved these changes Dec 2, 2025

View reviewed changes

pkg/gateway/sig/v4.go

return errors.ErrRequestNotReadyYet

}

// Calculate expiration from the signed time, not current time

Copy link

Member

N-o-Z Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽

zubron merged commit 92966ae into master Dec 2, 2025
144 of 148 checks passed

zubron deleted the fix/presigned-urls-ignore-expiration-9599 branch December 2, 2025 17:39

Add expiry validation for S3 gateway requests #9710

Add expiry validation for S3 gateway requests #9710

Conversation

zubron commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Description

Bug Fix

Testing Details

Breaking Change?

Uh oh!

N-o-Z left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zubron Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zubron commented Dec 2, 2025

Uh oh!

N-o-Z left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

N-o-Z commented Dec 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zubron commented Nov 24, 2025 •

edited

Loading

zubron Dec 2, 2025 •

edited

Loading