Fix signature v2 and presigned query unescaping. (#4936)

Simplifies the testing code by using s3signer
package from minio-go library.

Fixes #4927

cmd/bucket-handlers_test.go

@@ -455,7 +455,6 @@ func testListMultipartUploadsHandler(obj ObjectLayer, instanceType, bucketName s
 
 		// verify response for V2 signed HTTP request.
 		reqV2, err := newTestSignedRequestV2("GET", u, 0, nil, testCase.accessKey, testCase.secretKey)
-
 		if err != nil {
 			t.Fatalf("Test %d: %s: Failed to create HTTP request for PutBucketPolicyHandler: <ERROR> %v", i+1, instanceType, err)
 		}

cmd/object-handlers_test.go

@@ -2813,6 +2813,7 @@ func testAPIPutObjectPartHandlerPreSign(obj ObjectLayer, instanceType, bucketNam
 		t.Fatalf("[%s] - Failed to create an unsigned request to put object part for %s/%s <ERROR> %v",
 			instanceType, bucketName, testObject, err)
 	}
+
 	err = preSignV2(req, credentials.AccessKey, credentials.SecretKey, int64(10*60*60))
 	if err != nil {
 		t.Fatalf("[%s] - Failed to presign an unsigned request to put object part for %s/%s <ERROR> %v",
@@ -3301,6 +3302,7 @@ func testAPIListObjectPartsHandlerPreSign(obj ObjectLayer, instanceType, bucketN
 			instanceType, bucketName, mpartResp.UploadID)
 	}
 
+	req.Header = http.Header{}
 	err = preSignV2(req, credentials.AccessKey, credentials.SecretKey, int64(10*60*60))
 	if err != nil {
 		t.Fatalf("[%s] - Failed to presignV2 an unsigned request to list object parts for bucket %s, uploadId %s",

cmd/signature-v2.go

@@ -77,6 +77,20 @@ func doesPolicySignatureV2Match(formValues http.Header) APIErrorCode {
 	return ErrNone
 }
 
+// Escape encodedQuery string into unescaped list of query params, returns error
+// if any while unescaping the values.
+func unescapeQueries(encodedQuery string) (unescapedQueries []string, err error) {
+	for _, query := range strings.Split(encodedQuery, "&") {
+		var unescapedQuery string
+		unescapedQuery, err = url.QueryUnescape(query)
+		if err != nil {
+			return nil, err
+		}
+		unescapedQueries = append(unescapedQueries, unescapedQuery)
+	}
+	return unescapedQueries, nil
+}
+
 // doesPresignV2SignatureMatch - Verify query headers with presigned signature
 //     - http://docs.aws.amazon.com/AmazonS3/latest/dev/RESTAuthentication.html#RESTAuthenticationQueryStringAuth
 // returns ErrNone if matches. S3 errors otherwise.
@@ -92,38 +106,38 @@ func doesPresignV2SignatureMatch(r *http.Request) APIErrorCode {
 		encodedQuery = tokens[1]
 	}
 
-	queries := strings.Split(encodedQuery, "&")
+	var (
-	var filteredQueries []string
+		filteredQueries []string
-	var gotSignature string
+		gotSignature    string
-	var expires string
+		expires         string
-	var accessKey string
+		accessKey       string
-	var err error
+		err             error
-	for _, query := range queries {
+	)
-		keyval := strings.Split(query, "=")
+
+	var unescapedQueries []string
+	unescapedQueries, err = unescapeQueries(encodedQuery)
+	if err != nil {
+		errorIf(err, "Unable to unescape (%s)", encodedQuery)
+		return ErrInvalidQueryParams
+	}
+
+	// Extract the necessary values from presigned query, construct a list of new filtered queries.
+	for _, query := range unescapedQueries {
+		keyval := strings.SplitN(query, "=", 2)
 		switch keyval[0] {
 		case "AWSAccessKeyId":
-			accessKey, err = url.QueryUnescape(keyval[1])
+			accessKey = keyval[1]
 		case "Signature":
-			gotSignature, err = url.QueryUnescape(keyval[1])
+			gotSignature = keyval[1]
 		case "Expires":
-			expires, err = url.QueryUnescape(keyval[1])
+			expires = keyval[1]
 		default:
-			unescapedQuery, qerr := url.QueryUnescape(query)
+			filteredQueries = append(filteredQueries, query)
-			if qerr == nil {
-				filteredQueries = append(filteredQueries, unescapedQuery)
-			} else {
-				err = qerr
-			}
-		}
-		// Check if the query unescaped properly.
-		if err != nil {
-			errorIf(err, "Unable to unescape query values", queries)
-			return ErrInvalidQueryParams
 		}
 	}
 
-	// Invalid access key.
+	// Invalid values returns error.
-	if accessKey == "" {
+	if accessKey == "" || gotSignature == "" || expires == "" {
 		return ErrInvalidQueryParams
 	}
 
@@ -217,7 +231,13 @@ func doesSignV2Match(r *http.Request) APIErrorCode {
 		encodedQuery = tokens[1]
 	}
 
-	expectedAuth := signatureV2(r.Method, encodedResource, encodedQuery, r.Header)
+	unescapedQueries, err := unescapeQueries(encodedQuery)
+	if err != nil {
+		errorIf(err, "Unable to unescape (%s)", encodedQuery)
+		return ErrInvalidQueryParams
+	}
+
+	expectedAuth := signatureV2(r.Method, encodedResource, strings.Join(unescapedQueries, "&"), r.Header)
 	if v2Auth != expectedAuth {
 		return ErrSignatureDoesNotMatch
 	}
@@ -234,14 +254,14 @@ func calculateSignatureV2(stringToSign string, secret string) string {
 // Return signature-v2 for the presigned request.
 func preSignatureV2(method string, encodedResource string, encodedQuery string, headers http.Header, expires string) string {
 	cred := serverConfig.GetCredential()
-	stringToSign := presignV2STS(method, encodedResource, encodedQuery, headers, expires)
+	stringToSign := getStringToSignV2(method, encodedResource, encodedQuery, headers, expires)
 	return calculateSignatureV2(stringToSign, cred.SecretKey)
 }
 
 // Return signature-v2 authrization header.
 func signatureV2(method string, encodedResource string, encodedQuery string, headers http.Header) string {
 	cred := serverConfig.GetCredential()
-	stringToSign := signV2STS(method, encodedResource, encodedQuery, headers)
+	stringToSign := getStringToSignV2(method, encodedResource, encodedQuery, headers, "")
 	signature := calculateSignatureV2(stringToSign, cred.SecretKey)
 	return fmt.Sprintf("%s %s:%s", signV2Algorithm, cred.AccessKey, signature)
 }
@@ -267,7 +287,7 @@ func canonicalizedAmzHeadersV2(headers http.Header) string {
 }
 
 // Return canonical resource string.
-func canonicalizedResourceV2(encodedPath string, encodedQuery string) string {
+func canonicalizedResourceV2(encodedQuery string) string {
 	queries := strings.Split(encodedQuery, "&")
 	keyval := make(map[string]string)
 	for _, query := range queries {
@@ -280,6 +300,7 @@ func canonicalizedResourceV2(encodedPath string, encodedQuery string) string {
 		}
 		keyval[key] = val
 	}
+
 	var canonicalQueries []string
 	for _, key := range resourceList {
 		val, ok := keyval[key]
@@ -290,68 +311,55 @@ func canonicalizedResourceV2(encodedPath string, encodedQuery string) string {
 			canonicalQueries = append(canonicalQueries, key)
 			continue
 		}
-		// Resources values should be unescaped
+		canonicalQueries = append(canonicalQueries, key+"="+val)
-		unescapedVal, err := url.QueryUnescape(val)
-		if err != nil {
-			errorIf(err, "Unable to unescape query value (query = `%s`, value = `%s`)", key, val)
-			continue
-		}
-		canonicalQueries = append(canonicalQueries, key+"="+unescapedVal)
 	}
-	if len(canonicalQueries) == 0 {
+
-		return encodedPath
+	// The queries will be already sorted as resourceList is sorted, if canonicalQueries
-	}
+	// is empty strings.Join returns empty.
-	// the queries will be already sorted as resourceList is sorted.
+	return strings.Join(canonicalQueries, "&")
-	return encodedPath + "?" + strings.Join(canonicalQueries, "&")
 }
 
-// Return string to sign for authz header calculation.
+// Return string to sign under two different conditions.
-func signV2STS(method string, encodedResource string, encodedQuery string, headers http.Header) string {
+// - if expires string is set then string to sign includes date instead of the Date header.
+// - if expires string is empty then string to sign includes date header instead.
+func getStringToSignV2(method string, encodedResource, encodedQuery string, headers http.Header, expires string) string {
 	canonicalHeaders := canonicalizedAmzHeadersV2(headers)
 	if len(canonicalHeaders) > 0 {
 		canonicalHeaders += "\n"
 	}
 
+	date := expires // Date is set to expires date for presign operations.
+	if date == "" {
+		// If expires date is empty then request header Date is used.
+		date = headers.Get("Date")
+	}
+
 	// From the Amazon docs:
 	//
 	// StringToSign = HTTP-Verb + "\n" +
 	// 	 Content-Md5 + "\n" +
 	//	 Content-Type + "\n" +
-	//	 Date + "\n" +
+	//	 Date/Expires + "\n" +
 	//	 CanonicalizedProtocolHeaders +
 	//	 CanonicalizedResource;
 	stringToSign := strings.Join([]string{
 		method,
 		headers.Get("Content-MD5"),
 		headers.Get("Content-Type"),
-		headers.Get("Date"),
+		date,
 		canonicalHeaders,
-	}, "\n") + canonicalizedResourceV2(encodedResource, encodedQuery)
+	}, "\n")
 
-	return stringToSign
+	// For presigned signature no need to filter out based on resourceList,
-}
+	// just sign whatever is with the request.
-
+	if expires != "" {
-// Return string to sign for pre-sign signature calculation.
+		return stringToSign + encodedResource + "?" + encodedQuery
-func presignV2STS(method string, encodedResource string, encodedQuery string, headers http.Header, expires string) string {
-	canonicalHeaders := canonicalizedAmzHeadersV2(headers)
-	if len(canonicalHeaders) > 0 {
-		canonicalHeaders += "\n"
 	}
 
-	// From the Amazon docs:
+	canonicalResource := canonicalizedResourceV2(encodedQuery)
-	//
+	if canonicalResource != "" {
-	// StringToSign = HTTP-Verb + "\n" +
+		return stringToSign + encodedResource + "?" + canonicalResource
-	// 	 Content-Md5 + "\n" +
+	}
-	//	 Content-Type + "\n" +
+
-	//	 Expires + "\n" +
+	return stringToSign + encodedResource
-	//	 CanonicalizedProtocolHeaders +
-	//	 CanonicalizedResource;
-	stringToSign := strings.Join([]string{
-		method,
-		headers.Get("Content-MD5"),
-		headers.Get("Content-Type"),
-		expires,
-		canonicalHeaders,
-	}, "\n") + canonicalizedResourceV2(encodedResource, encodedQuery)
-	return stringToSign
 }

cmd/signature-v2_test.go

@@ -48,9 +48,12 @@ func TestDoesPresignedV2SignatureMatch(t *testing.T) {
 
 	now := UTCNow()
 
+	var (
+		accessKey = serverConfig.GetCredential().AccessKey
+		secretKey = serverConfig.GetCredential().SecretKey
+	)
 	testCases := []struct {
 		queryParams map[string]string
-		headers     map[string]string
 		expected    APIErrorCode
 	}{
 		// (0) Should error without a set URL query.
@@ -71,7 +74,7 @@ func TestDoesPresignedV2SignatureMatch(t *testing.T) {
 			queryParams: map[string]string{
 				"Expires":        "60s",
 				"Signature":      "badsignature",
-				"AWSAccessKeyId": serverConfig.GetCredential().AccessKey,
+				"AWSAccessKeyId": accessKey,
 			},
 			expected: ErrMalformedExpires,
 		},
@@ -80,7 +83,7 @@ func TestDoesPresignedV2SignatureMatch(t *testing.T) {
 			queryParams: map[string]string{
 				"Expires":        "60",
 				"Signature":      "badsignature",
-				"AWSAccessKeyId": serverConfig.GetCredential().AccessKey,
+				"AWSAccessKeyId": accessKey,
 			},
 			expected: ErrExpiredPresignRequest,
 		},
@@ -89,7 +92,7 @@ func TestDoesPresignedV2SignatureMatch(t *testing.T) {
 			queryParams: map[string]string{
 				"Expires":        fmt.Sprintf("%d", now.Unix()+60),
 				"Signature":      "badsignature",
-				"AWSAccessKeyId": serverConfig.GetCredential().AccessKey,
+				"AWSAccessKeyId": accessKey,
 			},
 			expected: ErrSignatureDoesNotMatch,
 		},
@@ -98,10 +101,17 @@ func TestDoesPresignedV2SignatureMatch(t *testing.T) {
 			queryParams: map[string]string{
 				"Expires":        fmt.Sprintf("%d", now.Unix()+60),
 				"Signature":      "zOM2YrY/yAQe15VWmT78OlBrK6g=",
-				"AWSAccessKeyId": serverConfig.GetCredential().AccessKey,
+				"AWSAccessKeyId": accessKey,
 			},
 			expected: ErrSignatureDoesNotMatch,
 		},
+		// (6) Should error when the signature does not match.
+		{
+			queryParams: map[string]string{
+				"response-content-disposition": "attachment; filename=\"4K%2d4M.txt\"",
+			},
+			expected: ErrNone,
+		},
 	}
 
 	// Run each test case individually.
@@ -111,25 +121,32 @@ func TestDoesPresignedV2SignatureMatch(t *testing.T) {
 		for key, value := range testCase.queryParams {
 			query.Set(key, value)
 		}
-
 		// Create a request to use.
-		req, e := http.NewRequest(http.MethodGet, "http://host/a/b?"+query.Encode(), nil)
+		req, err := http.NewRequest(http.MethodGet, "http://host/a/b?"+query.Encode(), nil)
-		if e != nil {
+		if err != nil {
-			t.Errorf("(%d) failed to create http.Request, got %v", i, e)
+			t.Errorf("(%d) failed to create http.Request, got %v", i, err)
 		}
-		// Should be set since we are simulating a http server.
+		if testCase.expected != ErrNone {
-		req.RequestURI = req.URL.RequestURI()
+			// Should be set since we are simulating a http server.
-
+			req.RequestURI = req.URL.RequestURI()
-		// Do the same for the headers.
+			// Check if it matches!
-		for key, value := range testCase.headers {
+			errCode := doesPresignV2SignatureMatch(req)
-			req.Header.Set(key, value)
+			if errCode != testCase.expected {
+				t.Errorf("(%d) expected to get %s, instead got %s", i, niceError(testCase.expected), niceError(errCode))
+			}
+		} else {
+			err = preSignV2(req, accessKey, secretKey, now.Unix()+60)
+			if err != nil {
+				t.Fatalf("(%d) failed to preSignV2 http request, got %v", i, err)
+			}
+			// Should be set since we are simulating a http server.
+			req.RequestURI = req.URL.RequestURI()
+			errCode := doesPresignV2SignatureMatch(req)
+			if errCode != testCase.expected {
+				t.Errorf("(%d) expected to get success, instead got %s", i, niceError(errCode))
+			}
 		}
 
-		// Check if it matches!
-		err := doesPresignV2SignatureMatch(req)
-		if err != testCase.expected {
-			t.Errorf("(%d) expected to get %s, instead got %s", i, niceError(testCase.expected), niceError(err))
-		}
 	}
 }
 

cmd/test-utils_test.go

@@ -20,14 +20,11 @@ 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"
@@ -52,6 +49,7 @@ import (
 
 	"github.com/fatih/color"
 	router "github.com/gorilla/mux"
+	"github.com/minio/minio-go/pkg/s3signer"
 )
 
 // Tests should initNSLock only once.
@@ -882,84 +880,14 @@ 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")
 	}
-
-	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.Split(req.URL.Path, "?")
-		if len(splits) > 0 {
-			encodedResource = splits[0]
-		}
-	}
-
-	// Get presigned string to sign.
-	stringToSign := presignV2STS(req.Method, encodedResource, encodedQuery, 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)
-
 	// Success.
+	req = s3signer.PreSignV2(*req, accessKeyID, secretAccessKey, expires)
 	return nil
 }
 
 // Sign given request using Signature V2.
 func signRequestV2(req *http.Request, accessKey, secretKey string) error {
-	// Initial time.
+	req = s3signer.SignV2(*req, accessKey, secretKey)
-	d := UTCNow()
-
-	// Add date if not present.
-	if date := req.Header.Get("Date"); date == "" {
-		req.Header.Set("Date", d.Format(http.TimeFormat))
-	}
-
-	splits := strings.Split(req.URL.Path, "?")
-	var encodedResource string
-	if len(splits) > 0 {
-		encodedResource = getURLEncodedName(splits[0])
-	}
-	encodedQuery := req.URL.Query().Encode()
-
-	// Calculate HMAC for secretAccessKey.
-	stringToSign := signV2STS(req.Method, encodedResource, encodedQuery, req.Header)
-	hm := hmac.New(sha1.New, []byte(secretKey))
-	hm.Write([]byte(stringToSign))
-
-	// Prepare auth header.
-	authHeader := new(bytes.Buffer)
-	authHeader.WriteString(fmt.Sprintf("%s %s:", signV2Algorithm, accessKey))
-	encoder := base64.NewEncoder(base64.StdEncoding, authHeader)
-	encoder.Write(hm.Sum(nil))
-	encoder.Close()
-
-	// Set Authorization header.
-	req.Header.Set("Authorization", authHeader.String())
 	return nil
 }