Skip to content

Commit

Permalink
aws/signer/v4: Fix Signing Unordered Multi Value Query Parameters (aw…
Browse files Browse the repository at this point in the history
…s#1491)

Removes sorting of query string values when calculating v4 signing as this is not part of the spec:
http://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html

The spec only requires the keys, not values, to be sorted which is achieved by Query.Encode().

Also fixed TestBuildCanonicalRequest, which was incorrectly testing without this important part of the build process leading to a false success.
  • Loading branch information
stevenh authored and jasdel committed Aug 28, 2017
1 parent 47416a1 commit 39a358f
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 8 deletions.
4 changes: 0 additions & 4 deletions aws/signer/v4/v4.go
Original file line number Diff line number Diff line change
Expand Up @@ -324,10 +324,6 @@ func (v4 Signer) signWithBody(r *http.Request, body io.ReadSeeker, service, regi
unsignedPayload: v4.UnsignedPayload,
}

for key := range ctx.Query {
sort.Strings(ctx.Query[key])
}

if ctx.isRequestSigned() {
ctx.Time = currentTimeFn()
ctx.handlePresignRemoval()
Expand Down
8 changes: 4 additions & 4 deletions aws/signer/v4/v4_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,14 @@ func TestPresignRequest(t *testing.T) {

func TestPresignBodyWithArrayRequest(t *testing.T) {
req, body := buildRequest("dynamodb", "us-east-1", "{}")
req.URL.RawQuery = "Foo=z&Foo=o&Foo=m&Foo=a"
req.URL.RawQuery = "FooB=a&FooA=b&FooA=a&FooB=b"

signer := buildSigner()
signer.Presign(req, body, "dynamodb", "us-east-1", 300*time.Second, time.Unix(0, 0))

expectedDate := "19700101T000000Z"
expectedHeaders := "content-length;content-type;host;x-amz-meta-other-header;x-amz-meta-other-header_with_underscore"
expectedSig := "fef6002062400bbf526d70f1a6456abc0fb2e213fe1416012737eebd42a62924"
expectedSig := "60152079d308a87f036082a236c935261221a32d369ec4b6b75dbc9873d2ab9a"
expectedCred := "AKID/19700101/us-east-1/dynamodb/aws4_request"
expectedTarget := "prefix.Operation"

Expand Down Expand Up @@ -519,7 +519,7 @@ func TestSignWithRequestBody_Overwrite(t *testing.T) {

func TestBuildCanonicalRequest(t *testing.T) {
req, body := buildRequest("dynamodb", "us-east-1", "{}")
req.URL.RawQuery = "Foo=z&Foo=o&Foo=m&Foo=a"
req.URL.RawQuery = "FooB=a&FooA=b&FooA=a&FooB=b"
ctx := &signingCtx{
ServiceName: "dynamodb",
Region: "us-east-1",
Expand All @@ -531,7 +531,7 @@ func TestBuildCanonicalRequest(t *testing.T) {
}

ctx.buildCanonicalString()
expected := "https://example.org/bucket/key-._~,!@#$%^&*()?Foo=z&Foo=o&Foo=m&Foo=a"
expected := "https://example.org/bucket/key-._~,!@#$%^&*()?FooA=b&FooA=a&FooB=a&FooB=b"
if e, a := expected, ctx.Request.URL.String(); e != a {
t.Errorf("expect %v, got %v", e, a)
}
Expand Down

0 comments on commit 39a358f

Please sign in to comment.