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

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

Closed
2 of 3 tasks
tqh opened this issue Oct 10, 2024 · 4 comments
Closed
2 of 3 tasks
Assignees
Labels
bug This issue is a bug. p3 This is a minor priority issue queued This issues is on the AWS team's backlog

Comments

@tqh
Copy link

tqh commented Oct 10, 2024

Acknowledgements

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

@tqh tqh added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 10, 2024
@RanVaknin RanVaknin self-assigned this Oct 10, 2024
@RanVaknin
Copy link
Contributor

RanVaknin commented Oct 10, 2024

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~

@RanVaknin RanVaknin added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. p3 This is a minor priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Oct 10, 2024
@tqh
Copy link
Author

tqh commented Oct 10, 2024

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
Copy link
Contributor

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.

@RanVaknin RanVaknin added queued This issues is on the AWS team's backlog and removed response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Oct 10, 2024
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p3 This is a minor priority issue queued This issues is on the AWS team's backlog
Projects
None yet
Development

No branches or pull requests

3 participants