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.59k stars 626 forks source link

PutObject does not add Content-Length in the headers when it is specified in the parameters and the body is not Seekable #2511

Closed riftsin closed 7 months ago

riftsin commented 7 months ago

Describe the bug

Hello,

When you do a PutObject like this:

buf := []byte{"just some random data"}
r, w := io.Pipe()
defer w.Close()
go func() {
                defer r.Close()
                _, err := s3Client.PutObject(context.TODO(), &s3.PutObjectInput{
                    Bucket:        aws.String("bucket"),
                    Key:           aws.String("screenshot.png"),
                    Body:          r,
                    ContentLength: aws.Int64(len(buf)),
                },
                    s3Client.optionFunc,
                    s3.WithAPIOptions(v4.SwapComputePayloadSHA256ForUnsignedPayloadMiddleware),
                )
                if err != nil {
                  log.Println(err)
                }
}()
io.Copy(w, bytes.NewReader(buf))

You get a SignatureDoesMatchError.

The reason for this is that the header Content-Length is not set in the Request as you can see in the following logs (even though it is set in the canonical request)

---[ CANONICAL STRING  ]-----------------------------
PUT
/bucket/screenshot.png
x-id=PutObject
accept-encoding:identity
amz-sdk-invocation-id:471a9f15-9cbc-4ade-a909-e15f847a4741
amz-sdk-request:attempt=1; max=1
content-length:172595
content-type:application/octet-stream
host:s3.eu-west-3.amazonaws.com
x-amz-content-sha256:UNSIGNED-PAYLOAD
x-amz-date:20240221T103126Z

accept-encoding;amz-sdk-invocation-id;amz-sdk-request;content-length;content-type;host;x-amz-content-sha256;x-amz-date
UNSIGNED-PAYLOAD
---[ STRING TO SIGN ]--------------------------------
AWS4-HMAC-SHA256
20240221T103126Z
20240221/eu-west-3/s3/aws4_request
3060f414e9bfb0d526ba52e0e5c5f889f37bb61005017d58804be9d38d0b657c
-----------------------------------------------------
SDK 2024/02/21 11:31:26 DEBUG Request
PUT /bucket/screenshot.png?x-id=PutObject HTTP/1.1
Host: s3.eu-west-3.amazonaws.com
User-Agent: aws-sdk-go-v2/1.21.2 os/macos lang/go#1.22.0 md/GOOS#darwin md/GOARCH#arm64 api/s3#1.38.5
Transfer-Encoding: chunked
Accept-Encoding: identity
Amz-Sdk-Invocation-Id: 471a9f15-9cbc-4ade-a909-e15f847a4741
Amz-Sdk-Request: attempt=1; max=1
Authorization: AWS4-HMAC-SHA256 Credential=AKIA***/20240221/eu-west-3/s3/aws4_request, SignedHeaders=accept-encoding;amz-sdk-invocation-id;amz-sdk-request;content-length;content-type;host;x-amz-content-sha256;x-amz-date, Signature=e0ebe472d82ea11f46d2dd02c74055f5ec81e9ddf66ca69cd4211248649c3a31
Content-Type: application/octet-stream
X-Amz-Content-Sha256: UNSIGNED-PAYLOAD
X-Amz-Date: 20240221T103126Z

Expected Behavior

I would expect the content length to be set when I specifiy it using the Content-Length parameter of PutObject.

Current Behavior

The Content-Length header is not set and therefore the request fails with a SignatureDoesNotMatch error.

Reproduction Steps

Just use the code in the example (left out the boiler plate)

Possible Solution

No response

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

In the example this was used

    github.com/aws/aws-sdk-go-v2 v1.21.2
    github.com/aws/aws-sdk-go-v2/config v1.18.38
    github.com/aws/aws-sdk-go-v2/credentials v1.13.36
    github.com/aws/aws-sdk-go-v2/service/s3 v1.38.5

However I have tested it with aws-sdk-go-v2 v1.25.0 and I get the same behavior

Compiler and Version used

1.22 darwin/arm64

Operating System and version

MacOS Ventura

lucix-aws commented 7 months ago

PutObject does not add Content-Length in the headers when it is specified in the parameters and the body is not Seekable

I don't think this is the case. Consider the following snippet, built with the express purpose of testing this claim (based heavily on what you provided, but removing the asynchronous aspect):

package main

import (
    "context"
    "io"
    "log"

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

type reader string

var _ io.Reader = reader("")

func (r reader) Read(p []byte) (int, error) {
    copy(p, r)
    return len(r), nil
}

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

    s3Client := s3.NewFromConfig(cfg, func(o *s3.Options) {
        o.ClientLogMode = aws.LogRequest | aws.LogResponse
    })

    str := reader("just some random data")
    _, err = s3Client.PutObject(context.TODO(), &s3.PutObjectInput{
        Bucket:        aws.String("<bucket>"),
        Key:           aws.String("screenshot.png"),
        Body:          str,
        ContentLength: aws.Int64(int64(len(str))),
    }, s3.WithAPIOptions(v4.SwapComputePayloadSHA256ForUnsignedPayloadMiddleware))
    if err != nil {
        log.Println(err)
    }
}

I'm seeing this upload successfully (with the content-length header included in the request) against the latest modules.

I have a few followup asks, related to your snippet as posted:

1. We can't see the source of s3Client.optionFunc, which may very well matter. Given its unexportedness I assume you've wrapped our client with your own structure that has this field. 2. You mentioned you removed some boilerplate, but your snippet as it's presented here isn't really functional because the inner goroutine that calls the operation is basically guaranteed to get dropped before it completes, since the outer main is going to return first. I can only assume there's other things going on in your application surrounding this, which are almost certainly going to be relevant as far as root-causing this is concerned.

lucix-aws commented 7 months ago

I missed the chunked Transfer-Encoding on the first pass. With some basic WaitGroup ops surrounding your snippet I can reproduce this.

We appear to have a special case internally for io.PipeReader that is causing us to send invalid signatures with this request configuration.

Since we've demonstrated that content-length + non-seekable body does work in the general sense, I'm going to close this issue and open a new one to track the PipeReader issue separately.

github-actions[bot] commented 7 months 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.