aws/signer/v4/middleware.go: GetSignedRequestSignature cuts Signature= wrong for indexes > 0

tqh commented 1 month ago

Acknowledgements

[X] I have searched (https://github.com/aws/aws-sdk/issues?q=is%3Aissue) for past instances of this issue
[X] I have verified all of my SDK modules are up-to-date (you can perform a bulk update with go get -u github.com/aws/aws-sdk-go-v2/...)

Describe the bug

Some s3 clients, like s3cmd, does not have Signature= directly after ,.

While GetSignedRequestSignature tries to handle that case with idx >= 0 it does not use idx when cutting the string. This causes the result to contain random non hex data in sig and you get an error about invalid hex: encoding/hex: invalid byte: U+0053 'S'

Regression Issue

[ ] Select this option if this issue appears to be a regression.

Expected Behavior

GetSignedRequestSignature(r *http.Request) to return a signature

Current Behavior

GetSignedRequestSignature(r *http.Request) returns error encoding/hex: invalid byte: U+0053 'S'

Reproduction Steps

Using s3cmd ls for a service that uses validation

Possible Solution

Missing index when cutting string sig := p[len(authHeaderSignatureElem):] should be sig := p[len(authHeaderSignatureElem) + idx:]

Additional Information/Context

Confirmed that adding idx as suggested solves the issue locally.

AWS Go SDK V2 Module Versions Used

v1.32.2

Compiler and Version used

go version go1.22.5 linux/amd64

Operating System and version

Linux amd64

RanVaknin commented 1 month ago

Hi @tqh ,

Can you please give us some more details about how this is broken? like log the signature that does not get extracted? The AWS SDK only officially supports AWS offerings. s3cmd is a 3rd party tool so we are not familiar with their innerworkings and how they build the request. If they format their request object differently than how the SDK does, then it will likely not work.

Can you please provide additional info so we may take a closer look?

Thanks, Ran~

tqh commented 1 month ago

Here is a sample which prints [] encoding/hex: invalid byte: U+0053 'S':

package main

import (
        "fmt"
        "net/http"

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

func main() {
        r, err := http.NewRequest("GET", "/", nil)
        if err != nil {
                panic(err)
        }
        r.Header.Add("Authorization", "AWS4-HMAC-SHA256 Credential=REMOVED/20241010/US/s3/aws4_request,SignedHeaders=host;x-amz-content-sha256;x-amz-date,Signature=CAFECAFE")
        sig, err := v4.GetSignedRequestSignature(r)
        fmt.Println(sig, err)
}

lucix-aws commented 1 month ago

For the record, the IAM docs on sigv4 don't say much about whitespace or lack thereof, but I just checked against SQS and they allow any or no whitespace around the components of the signature they expect.

e.g. all of these are accepted by SQS

"%s Credential=%s, SignedHeaders=%s, Signature=%s"
"%s Credential=%s,SignedHeaders=%s,Signature=%s"
"%s Credential=%s,                      \t       SignedHeaders=%s,\t       Signature=%s         "

So yes, we can definitely improve the parsing logic a bit here.

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

aws / aws-sdk-go-v2