From 39a358f9d642fe3804762f998b8cfbe46ba5a61a Mon Sep 17 00:00:00 2001 From: Steven Hartland Date: Mon, 28 Aug 2017 23:31:56 +0100 Subject: [PATCH] aws/signer/v4: Fix Signing Unordered Multi Value Query Parameters (#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. --- aws/signer/v4/v4.go | 4 ---- aws/signer/v4/v4_test.go | 8 ++++---- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/aws/signer/v4/v4.go b/aws/signer/v4/v4.go index 15da57249a1..cdfa22d5aad 100644 --- a/aws/signer/v4/v4.go +++ b/aws/signer/v4/v4.go @@ -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() diff --git a/aws/signer/v4/v4_test.go b/aws/signer/v4/v4_test.go index 45d0eb8869a..178ad816596 100644 --- a/aws/signer/v4/v4_test.go +++ b/aws/signer/v4/v4_test.go @@ -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" @@ -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", @@ -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) }