From 6dcfaa877cfe6d1b093ae05abd210a3218d81618 Mon Sep 17 00:00:00 2001
From: Harshavardhana <harsha@minio.io>
Date: Tue, 26 Sep 2017 11:00:07 -0700
Subject: [PATCH] Fix signature v2 handling for resource names (#4965)

Previously we were wrongly adding `?` as part
of the resource name, add a test case to check
if this is handled properly.

Thanks to @kannappanr for reproducing this.

Without this change presigned URL generated with following
command would fail with signature mismatch.
```
aws s3 presign s3://testbucket/functional-tests.sh
```
---
 cmd/signature-v2.go      | 21 +++++----------
 cmd/signature-v2_test.go |  7 ++++-
 cmd/test-utils_test.go   | 55 ++++++++++++++++++++++++++++++++++++++--
 3 files changed, 66 insertions(+), 17 deletions(-)

diff --git a/cmd/signature-v2.go b/cmd/signature-v2.go
index adcbffcaf..71250abc5 100644
--- a/cmd/signature-v2.go
+++ b/cmd/signature-v2.go
@@ -287,7 +287,7 @@ func canonicalizedAmzHeadersV2(headers http.Header) string {
 }
 
 // Return canonical resource string.
-func canonicalizedResourceV2(encodedQuery string) string {
+func canonicalizedResourceV2(encodedResource, encodedQuery string) string {
 	queries := strings.Split(encodedQuery, "&")
 	keyval := make(map[string]string)
 	for _, query := range queries {
@@ -316,7 +316,11 @@ func canonicalizedResourceV2(encodedQuery string) string {
 
 	// The queries will be already sorted as resourceList is sorted, if canonicalQueries
 	// is empty strings.Join returns empty.
-	return strings.Join(canonicalQueries, "&")
+	canonicalQuery := strings.Join(canonicalQueries, "&")
+	if canonicalQuery != "" {
+		return encodedResource + "?" + canonicalQuery
+	}
+	return encodedResource
 }
 
 // Return string to sign under two different conditions.
@@ -350,16 +354,5 @@ func getStringToSignV2(method string, encodedResource, encodedQuery string, head
 		canonicalHeaders,
 	}, "\n")
 
-	// For presigned signature no need to filter out based on resourceList,
-	// just sign whatever is with the request.
-	if expires != "" {
-		return stringToSign + encodedResource + "?" + encodedQuery
-	}
-
-	canonicalResource := canonicalizedResourceV2(encodedQuery)
-	if canonicalResource != "" {
-		return stringToSign + encodedResource + "?" + canonicalResource
-	}
-
-	return stringToSign + encodedResource
+	return stringToSign + canonicalizedResourceV2(encodedResource, encodedQuery)
 }
diff --git a/cmd/signature-v2_test.go b/cmd/signature-v2_test.go
index 68913d887..7c8ab234f 100644
--- a/cmd/signature-v2_test.go
+++ b/cmd/signature-v2_test.go
@@ -105,13 +105,18 @@ func TestDoesPresignedV2SignatureMatch(t *testing.T) {
 			},
 			expected: ErrSignatureDoesNotMatch,
 		},
-		// (6) Should error when the signature does not match.
+		// (6) Should not error signature matches with extra query params.
 		{
 			queryParams: map[string]string{
 				"response-content-disposition": "attachment; filename=\"4K%2d4M.txt\"",
 			},
 			expected: ErrNone,
 		},
+		// (7) Should not error signature matches with no special query params.
+		{
+			queryParams: map[string]string{},
+			expected:    ErrNone,
+		},
 	}
 
 	// Run each test case individually.
diff --git a/cmd/test-utils_test.go b/cmd/test-utils_test.go
index 441ced98a..00481b40c 100644
--- a/cmd/test-utils_test.go
+++ b/cmd/test-utils_test.go
@@ -20,11 +20,14 @@ import (
 	"bufio"
 	"bytes"
 	"crypto/ecdsa"
+	"crypto/hmac"
 	crand "crypto/rand"
 	"crypto/rsa"
+	"crypto/sha1"
 	"crypto/tls"
 	"crypto/x509"
 	"crypto/x509/pkix"
+	"encoding/base64"
 	"encoding/hex"
 	"encoding/json"
 	"encoding/pem"
@@ -880,8 +883,56 @@ func preSignV2(req *http.Request, accessKeyID, secretAccessKey string, expires i
 	if accessKeyID == "" || secretAccessKey == "" {
 		return errors.New("Presign cannot be generated without access and secret keys")
 	}
-	// Success.
-	req = s3signer.PreSignV2(*req, accessKeyID, secretAccessKey, expires)
+
+	// FIXME: Remove following portion of code after fixing a bug in minio-go preSignV2.
+
+	d := UTCNow()
+	// Find epoch expires when the request will expire.
+	epochExpires := d.Unix() + expires
+
+	// Add expires header if not present.
+	expiresStr := req.Header.Get("Expires")
+	if expiresStr == "" {
+		expiresStr = strconv.FormatInt(epochExpires, 10)
+		req.Header.Set("Expires", expiresStr)
+	}
+
+	// url.RawPath will be valid if path has any encoded characters, if not it will
+	// be empty - in which case we need to consider url.Path (bug in net/http?)
+	encodedResource := req.URL.RawPath
+	encodedQuery := req.URL.RawQuery
+	if encodedResource == "" {
+		splits := strings.SplitN(req.URL.Path, "?", 2)
+		encodedResource = splits[0]
+		if len(splits) == 2 {
+			encodedQuery = splits[1]
+		}
+	}
+
+	unescapedQueries, err := unescapeQueries(encodedQuery)
+	if err != nil {
+		return err
+	}
+
+	// Get presigned string to sign.
+	stringToSign := getStringToSignV2(req.Method, encodedResource, strings.Join(unescapedQueries, "&"), req.Header, expiresStr)
+	hm := hmac.New(sha1.New, []byte(secretAccessKey))
+	hm.Write([]byte(stringToSign))
+
+	// Calculate signature.
+	signature := base64.StdEncoding.EncodeToString(hm.Sum(nil))
+
+	query := req.URL.Query()
+	// Handle specially for Google Cloud Storage.
+	query.Set("AWSAccessKeyId", accessKeyID)
+	// Fill in Expires for presigned query.
+	query.Set("Expires", strconv.FormatInt(epochExpires, 10))
+
+	// Encode query and save.
+	req.URL.RawQuery = query.Encode()
+
+	// Save signature finally.
+	req.URL.RawQuery += "&Signature=" + url.QueryEscape(signature)
 	return nil
 }