search

aws / aws-sdk-go-v2

AWS SDK for the Go programming language.
https://aws.github.io/aws-sdk-go-v2/docs/
Apache License 2.0
2.58k stars 623 forks source link

Presign url's require side loading request-payer header #2764

Closed Maldris closed 1 week ago

Maldris commented 2 weeks ago

Acknowledgements

Describe the bug

When presigning a object get request to service a thumbnail to a client, I attempt to sign the url like so

req, err := s3.NewPresignClient(s3Client).PresignGetObject(context.Background(), &s3.GetObjectInput{
    Bucket:              &u.Host,
    Key:                 &u.Path,
    RequestPayer:        types.RequestPayerRequester,
}, s3.WithPresignExpires(time.Minute))

This returns a url, and Signed header, with the client required to specify the X-Amz-Request-Payer: requester header.

Expected Behavior

Normally I would expect a signed URL to be relatively self contained for a GET operation and not require a specific header to be set if I redirect a client to a URL (and as the buckets differ by use case we can't simply set that for all clients).

Current Behavior

Instead, the client receives a

<Error>
<Code>SignatureDoesNotMatch</Code>
<Message>The request signature we calculated does not match the signature you provided. Check your key and signing method.</Message>
</Error>

As it did not contain the mandatory header.

Reproduction Steps

Using the USGS landsat bucket as an example:

package main

import (
    "context"
    "fmt"
    "time"

    "github.com/aws/aws-sdk-go-v2/aws"
    "github.com/aws/aws-sdk-go-v2/config"
    "github.com/aws/aws-sdk-go-v2/service/s3"
    "github.com/aws/aws-sdk-go-v2/service/s3/types"
)

func main() {
    cfg, err := config.LoadDefaultConfig(context.Background())
    if err != nil {
        panic(err)
    }

    svc := s3.NewPresignClient(s3.NewFromConfig(cfg))
    req, err := svc.PresignGetObject(context.Background(), &s3.GetObjectInput{
        Bucket: aws.String("usgs-landsat"),
        Key:    aws.String("collection02/level-2/standard/oli-tirs/2024/201/032/LC09_L2SP_201032_20240129_20240130_02_T1/LC09_L2SP_201032_20240129_20240130_02_T1_thumb_large.jpeg"),
        RequestPayer:        types.RequestPayerRequester,
    }, func(o *s3.PresignOptions) {
        o.Expires = 20 * time.Minute
    })
    if err != nil {
        panic(err)
    }

    fmt.Println(req.Method, req.URL, req.SignedHeader)
}

Possible Solution

This is very similar to the issues reported in

2484 #2508 and #2662

and similar changes may suffice

In that case, remove entry from the RequiredSignedHeaders map in aws/signer/internal/v4/headers.go This should allow the parameter to be signed as the x-amz-request-payer query parameter.

I'm aware that this query parameter is indeed supported by the server due to the work around used for a similar issue in V1 (see additional information). As a result, this change should work, but may also require the argument to be lower cased (I know some of these arguments will attempt to pass it as title case, and I'm unsure if the server accepts the argument in any case, or only lower).

Additional Information/Context

A similar issue was present in v1, but was able to be worked around by manipulating the request (no longer viable in v2)

An example of this would be:

params := &s3.GetObjectInput{
        Bucket:               aws.String(bucket),
        Key:                  aws.String(key),
}
req, _ := client.GetObjectRequest(params)
q := req.HTTPRequest.URL.Query()
if requestPayer {
    q.Set("x-amz-request-payer", "requester")
}
req.HTTPRequest.URL.RawQuery = q.Encode()
urlStr, err := req.Presign(time.Minute)
if err != nil {
    panic(err)
}

AWS Go SDK V2 Module Versions Used

bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go@v1.55.5 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2@v1.30.4 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream@v1.6.4 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/config@v1.27.30 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/credentials@v1.17.29 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/feature/ec2/imds@v1.16.12 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/feature/s3/manager@v1.17.14 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/internal/configsources@v1.3.16 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/internal/endpoints/v2@v2.6.16 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/internal/ini@v1.8.1 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/internal/v4a@v1.3.16 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding@v1.11.4 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/service/internal/checksum@v1.3.18 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/service/internal/presigned-url@v1.11.18 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/service/internal/s3shared@v1.17.16 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/service/kms@v1.35.5 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/service/s3@v1.60.1 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/service/secretsmanager@v1.32.6 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/service/sns@v1.31.5 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/service/sqs@v1.34.5 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/service/ssm@v1.52.6 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/service/sso@v1.22.5 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/service/ssooidc@v1.26.5 bitbucket.org/arlula/landsatIntegration github.com/aws/aws-sdk-go-v2/service/sts@v1.30.5

Compiler and Version used

1.23.0

Operating System and version

Windows 10

Maldris commented 2 weeks ago

I'm assuming that the triage team will be able to delete those bot posts, as I cannot

Maldris commented 2 weeks ago

also, I was reading the source further and realised, the Range header will have the same issue if you wanted to sign a request to a range within a file

again, this issue occurred in v1, our presign get object helper for v1 looked something like (still simplified as it had a bunch of debug reporting and extras not relevant here):

func GetS3RedirectForResource(bucket, key, name, contentType string, byteRange *Range, requestPayer bool, client *s3.S3) (string, error) {
    exp := time.Now().Add(time.Second * 15)
    params := &s3.GetObjectInput{
        Bucket:               aws.String(bucket),
        Key:                  aws.String(key),
        ResponseExpires:      &exp,
    }
    if name != "" {
        params.ResponseContentDisposition = aws.String(`attachment; filename="` + name + `"`)
    }
    if contentType != "" {
        params.ResponseContentType = aws.String(contentType)
    }
    req, _ := client.GetObjectRequest(params)
    q := req.HTTPRequest.URL.Query()
    if byteRange != nil {
        q.Set("range", byteRange.String())
    }
    if requestPayer {
        q.Set("x-amz-request-payer", "requester")
    }
    req.HTTPRequest.URL.RawQuery = q.Encode()
    urlStr, err := req.Presign(time.Minute)
    if err != nil {
        return "", errors.Wrap(err, "Error getting S3 presigned url")
    }
    return urlStr, nil
}

Where range was a helper decoder in our package for decoding range request headers for validation

RanVaknin commented 2 weeks ago

Hi @Maldris ,

In that case, remove entry from the RequiredSignedHeaders map in aws/signer/internal/v4/headers.go This should allow the parameter to be signed as the x-amz-request-payer query parameter.

Unlike expected-bucket-owner that was hoisted in v1, and signed in v2 (breaking change), this header you are describing did not change its behavior from v1 to v2, therefore we are not going to change its behavior at this point.

A similar issue was present in v1, but was able to be worked around by manipulating the request (no longer viable in v2)

You can implement a middleware that does the same thing in v2 by using the snippet from my comment here.

In that comment I also explain the rationale behind signing most things instead of hoisting. There are efforts from S3 to standardize the behavior of the presigned URLs, but without having the server side support we cannot change the implementation.

I understand that this is counter intuitive, and I just want to emphasize that we are aware this issue of hoisting vs signing in presigned URLs is a pain point across all SDKs. There are active efforts both from the SDK team and from S3 to address this, but we are unable to commit to an ETA.

Let me know if you are able to get that middleware to work.

Thanks, Ran~

github-actions[bot] commented 2 weeks ago

This issue is now closed. Comments on closed issues are hard for our team to see. If you need more assistance, please open a new issue that references this one.

Maldris commented 1 week ago

@RanVaknin, I'm sorry that this got closed over a weekend, as I don't believe this issue is actually completed.

Re the PR #2768 by @bhavya2109sharma, that PR doesn't appear to work, and now a request will not work even if a client sends the header.

The cause appears to be that the header uses the title case form X-Amz-Request-Payer, which is what that PR also uses. However, the server accepts the query parameter in lower case form x-amz-request-payer as shown in my example above (and the same for Range v range).

To address your comments however, as described above, these are features that the S3 server does support, and we actively use in one of our older systems (where that snippet I included above came from), so the comment:

but without having the server side support we cannot change the implementation.

is somewhat moot as the server does support both of these features.

As for your middleware suggestion, we are investigating if we can utilize it. However, as out main application is designed to deploy to multiple clouds, and these behaviours are mediated through a CDK to abstract cloud specific behaviour, this pattern is both harder to implement, and may put us in breach of our requirements for deploying to military and governmental clients (part of the reason we are reviewing and removing sdk v1 from our older stable systems).

I have a working prototype using this pattern for now, it involves quite a bit of a kludge to happen at all, but it does work, the telling thing will be if Platform One flags it, as it does breach their rules for hardened deployment images and coding standards.

github-actions[bot] commented 1 week ago

This issue is now closed. Comments on closed issues are hard for our team to see. If you need more assistance, please open a new issue that references this one.