From c3ff402fcb44cfab0a1bc4fbed8fb66ee292fa18 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Sun, 24 Sep 2017 14:20:12 -0700 Subject: [PATCH] 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 | 1 - cmd/object-handlers_test.go | 2 + cmd/signature-v2.go | 144 +++++++++++++++++++----------------- cmd/signature-v2_test.go | 57 +++++++++----- cmd/test-utils_test.go | 78 +------------------ 5 files changed, 118 insertions(+), 164 deletions(-) diff --git a/cmd/bucket-handlers_test.go b/cmd/bucket-handlers_test.go index 99fa1ad13..04e71416c 100644 --- a/cmd/bucket-handlers_test.go +++ b/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: %v", i+1, instanceType, err) } diff --git a/cmd/object-handlers_test.go b/cmd/object-handlers_test.go index 00f0e5992..a8e46c786 100644 --- a/cmd/object-handlers_test.go +++ b/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 %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 %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", diff --git a/cmd/signature-v2.go b/cmd/signature-v2.go index f38ec73cc..adcbffcaf 100644 --- a/cmd/signature-v2.go +++ b/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 filteredQueries []string - var gotSignature string - var expires string - var accessKey string - var err error - for _, query := range queries { - keyval := strings.Split(query, "=") + var ( + filteredQueries []string + gotSignature string + expires string + accessKey string + err error + ) + + 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) - 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 + filteredQueries = append(filteredQueries, query) } } - // Invalid access key. - if accessKey == "" { + // Invalid values returns error. + 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 - 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) + canonicalQueries = append(canonicalQueries, key+"="+val) } - if len(canonicalQueries) == 0 { - return encodedPath - } - // the queries will be already sorted as resourceList is sorted. - return encodedPath + "?" + strings.Join(canonicalQueries, "&") + + // The queries will be already sorted as resourceList is sorted, if canonicalQueries + // is empty strings.Join returns empty. + return strings.Join(canonicalQueries, "&") } -// Return string to sign for authz header calculation. -func signV2STS(method string, encodedResource string, encodedQuery string, headers http.Header) string { +// Return string to sign under two different conditions. +// - 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 stringToSign + encodedResource + "?" + encodedQuery + } -// Return string to sign for pre-sign signature calculation. -func presignV2STS(method string, encodedResource string, encodedQuery string, headers http.Header, expires string) string { - canonicalHeaders := canonicalizedAmzHeadersV2(headers) - if len(canonicalHeaders) > 0 { - canonicalHeaders += "\n" + canonicalResource := canonicalizedResourceV2(encodedQuery) + if canonicalResource != "" { + return stringToSign + encodedResource + "?" + canonicalResource } - // From the Amazon docs: - // - // StringToSign = HTTP-Verb + "\n" + - // Content-Md5 + "\n" + - // Content-Type + "\n" + - // Expires + "\n" + - // CanonicalizedProtocolHeaders + - // CanonicalizedResource; - stringToSign := strings.Join([]string{ - method, - headers.Get("Content-MD5"), - headers.Get("Content-Type"), - expires, - canonicalHeaders, - }, "\n") + canonicalizedResourceV2(encodedResource, encodedQuery) - return stringToSign + return stringToSign + encodedResource } diff --git a/cmd/signature-v2_test.go b/cmd/signature-v2_test.go index 1e9764be4..68913d887 100644 --- a/cmd/signature-v2_test.go +++ b/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) - if e != nil { - t.Errorf("(%d) failed to create http.Request, got %v", i, e) + req, err := http.NewRequest(http.MethodGet, "http://host/a/b?"+query.Encode(), nil) + if err != nil { + t.Errorf("(%d) failed to create http.Request, got %v", i, err) } - // Should be set since we are simulating a http server. - req.RequestURI = req.URL.RequestURI() - - // Do the same for the headers. - for key, value := range testCase.headers { - req.Header.Set(key, value) + if testCase.expected != ErrNone { + // Should be set since we are simulating a http server. + req.RequestURI = req.URL.RequestURI() + // Check if it matches! + errCode := doesPresignV2SignatureMatch(req) + 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)) - } } } diff --git a/cmd/test-utils_test.go b/cmd/test-utils_test.go index b339ef146..441ced98a 100644 --- a/cmd/test-utils_test.go +++ b/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. - 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()) + req = s3signer.SignV2(*req, accessKey, secretKey) return nil }