Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactoring of buildQuery to accept a list of maintained headers to l… #2773

Merged
merged 3 commits into from
Sep 3, 2024

Conversation

RanVaknin
Copy link
Contributor

@RanVaknin RanVaknin commented Sep 3, 2024

This PR supplements #2768
Following #2764
In order to correctly test this changes pertaining to this header, one must use 2 separate accounts.

Account A - where the bucket lives
Account B - that makes and signs the request going to that bucket in account A.

In order for this to be enforced, the bucket on account A must be configured to be a request payer bucket, meaning that whoever is making the request to the bucket is going to be billed for it.

Manual "live" testing before change:

https://testbucket-foo-REDACTED.s3.us-east-1.amazonaws.com/my-file.txt?
X-Amz-Credential=REDACTED/20240903/us-east-1/s3/aws4_request&
X-Amz-Date=20240903T164027Z&
X-Amz-Expires=2100&
X-Amz-Request-Payer=requester&
X-Amz-SignedHeaders=host&
x-id=GetObject&
X-Amz-Signature=REDACTED&
X-Amz-Algorithm=AWS4-HMAC-SHA256
403 Forbidden 403
<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>AccessDenied</Code><Message>Access Denied</Message><RequestId>REDACTED</RequestId><HostId>REDACTED</HostId></Error>

After change:

https://testbucket-foo-REDACTED.s3.us-east-1.amazonaws.com/my-file.txt?
X-Amz-Expires=2100&
X-Amz-SignedHeaders=host&
x-amz-request-payer=requester&
x-id=GetObject&
X-Amz-Signature=REDACTED&
X-Amz-Algorithm=AWS4-HMAC-SHA256&
X-Amz-Credential=REDACTED/20240903/us-east-1/s3/aws4_request&
X-Amz-Date=20240903T163943Z 
200 OK 200

@RanVaknin RanVaknin requested a review from a team as a code owner September 3, 2024 16:41
Copy link
Contributor

@Madrigal Madrigal left a comment

Choose a reason for hiding this comment

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

Not necessary to do on this iteration, but #2764 also mentioned

as shown in my example above (and the same for Range v range).

if we can confirm this to be true, we should also add range here

@lucix-aws lucix-aws merged commit 3120376 into aws:main Sep 3, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants