From a8ab02a73ab79753e8e35f35ab438f5ac3ec5eab Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Thu, 10 Nov 2016 21:57:15 -0800 Subject: [PATCH] v4/presign: Fix presign requests when there are more signed headers. (#3222) This fix removes a wrong logic which fails for requests which have more signed headers in a presign request. Fixes #3217 --- cmd/auth-handler.go | 3 +-- cmd/object-handlers_test.go | 42 ++++++++++++++++++++++++++++++--- cmd/signature-v4-parser.go | 5 ---- cmd/signature-v4-utils.go | 29 ++++++++++++----------- cmd/signature-v4_test.go | 46 ++++++++++++++++++++++++++----------- cmd/test-utils_test.go | 39 +++++++++++++++++++++++++++++++ cmd/web-handlers.go | 2 +- 7 files changed, 129 insertions(+), 37 deletions(-) diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go index 0982403e7..e5b346eba 100644 --- a/cmd/auth-handler.go +++ b/cmd/auth-handler.go @@ -163,9 +163,8 @@ func isReqAuthenticated(r *http.Request, region string) (s3Error APIErrorCode) { } // Populate back the payload. r.Body = ioutil.NopCloser(bytes.NewReader(payload)) + // Skips calculating sha256 on the payload on server, if client requested for it. var sha256sum string - // Skips calculating sha256 on the payload on server, - // if client requested for it. if skipContentSha256Cksum(r) { sha256sum = unsignedPayload } else { diff --git a/cmd/object-handlers_test.go b/cmd/object-handlers_test.go index c9c241d52..47fdab671 100644 --- a/cmd/object-handlers_test.go +++ b/cmd/object-handlers_test.go @@ -30,7 +30,6 @@ import ( "strconv" "sync" "testing" - "time" ) // Type to capture different modifications to API request to simulate failure cases. @@ -2111,7 +2110,24 @@ 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.AccessKeyID, credentials.SecretAccessKey, int64(10*time.Minute)) + err = preSignV2(req, credentials.AccessKeyID, credentials.SecretAccessKey, int64(10*60*60)) + if err != nil { + t.Fatalf("[%s] - Failed to presign an unsigned request to put object part for %s/%s %v", + instanceType, bucketName, testObject, err) + } + apiRouter.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Errorf("Test %d %s expected to succeed but failed with HTTP status code %d", 1, instanceType, rec.Code) + } + + rec = httptest.NewRecorder() + req, err = newTestRequest("PUT", getPutObjectPartURL("", bucketName, testObject, mpartResp.UploadID, "1"), + int64(len("hello")), bytes.NewReader([]byte("hello"))) + if err != nil { + t.Fatalf("[%s] - Failed to create an unsigned request to put object part for %s/%s %v", + instanceType, bucketName, testObject, err) + } + err = preSignV4(req, credentials.AccessKeyID, credentials.SecretAccessKey, int64(10*60*60)) if err != nil { t.Fatalf("[%s] - Failed to presign an unsigned request to put object part for %s/%s %v", instanceType, bucketName, testObject, err) @@ -2582,7 +2598,27 @@ func testAPIListObjectPartsHandlerPreSign(obj ObjectLayer, instanceType, bucketN instanceType, bucketName, mpartResp.UploadID) } - err = preSignV2(req, credentials.AccessKeyID, credentials.SecretAccessKey, int64(10*time.Minute)) + err = preSignV2(req, credentials.AccessKeyID, credentials.SecretAccessKey, 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", + instanceType, bucketName, mpartResp.UploadID) + } + apiRouter.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Errorf("Test %d %s expected to succeed but failed with HTTP status code %d", + 1, instanceType, rec.Code) + } + + rec = httptest.NewRecorder() + req, err = newTestRequest("GET", + getListMultipartURLWithParams("", bucketName, testObject, mpartResp.UploadID, "", "", ""), + 0, nil) + if err != nil { + t.Fatalf("[%s] - Failed to create an unsigned request to list object parts for bucket %s, uploadId %s", + instanceType, bucketName, mpartResp.UploadID) + } + + err = preSignV4(req, credentials.AccessKeyID, credentials.SecretAccessKey, 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", instanceType, bucketName, mpartResp.UploadID) diff --git a/cmd/signature-v4-parser.go b/cmd/signature-v4-parser.go index d1cc3610e..8b24bb22b 100644 --- a/cmd/signature-v4-parser.go +++ b/cmd/signature-v4-parser.go @@ -184,11 +184,6 @@ func parsePreSignV4(query url.Values) (preSignValues, APIErrorCode) { if err != ErrNone { return preSignValues{}, err } - // `host` is the only header used during the presigned request. - // Malformed signed headers has be caught here, otherwise it'll lead to signature mismatch. - if preSignV4Values.SignedHeaders[0] != "host" { - return preSignValues{}, ErrUnsignedHeaders - } // Save signature. preSignV4Values.Signature, err = parseSignature("Signature=" + query.Get("X-Amz-Signature")) diff --git a/cmd/signature-v4-utils.go b/cmd/signature-v4-utils.go index 7dbd32f05..f2a6cf49b 100644 --- a/cmd/signature-v4-utils.go +++ b/cmd/signature-v4-utils.go @@ -33,10 +33,17 @@ const unsignedPayload = "UNSIGNED-PAYLOAD" // http Header "x-amz-content-sha256" == "UNSIGNED-PAYLOAD" indicates that the // client did not calculate sha256 of the payload. Hence we skip calculating sha256. -// We also skip calculating sha256 for presigned requests without "x-amz-content-sha256" header. +// We also skip calculating sha256 for presigned requests without "x-amz-content-sha256" +// query header. func skipContentSha256Cksum(r *http.Request) bool { - contentSha256 := r.Header.Get("X-Amz-Content-Sha256") - return isRequestUnsignedPayload(r) || (isRequestPresignedSignatureV4(r) && contentSha256 == "") + queryContentSha256 := r.URL.Query().Get("X-Amz-Content-Sha256") + isRequestPresignedUnsignedPayload := func(r *http.Request) bool { + if isRequestPresignedSignatureV4(r) { + return queryContentSha256 == "" || queryContentSha256 == unsignedPayload + } + return false + } + return isRequestUnsignedPayload(r) || isRequestPresignedUnsignedPayload(r) } // isValidRegion - verify if incoming region value is valid with configured Region. @@ -99,27 +106,24 @@ func getURLEncodedName(name string) string { return encodedName } -// find whether "host" is part of list of signed headers. func findHost(signedHeaders []string) APIErrorCode { - for _, header := range signedHeaders { - if header == "host" { - return ErrNone - } + if contains(signedHeaders, "host") { + return ErrNone } return ErrUnsignedHeaders } // extractSignedHeaders extract signed headers from Authorization header func extractSignedHeaders(signedHeaders []string, reqHeaders http.Header) (http.Header, APIErrorCode) { - errCode := findHost(signedHeaders) - if errCode != ErrNone { - return nil, errCode + // find whether "host" is part of list of signed headers. + // if not return ErrUnsignedHeaders. "host" is mandatory. + if !contains(signedHeaders, "host") { + return nil, ErrUnsignedHeaders } extractedSignedHeaders := make(http.Header) for _, header := range signedHeaders { // `host` will not be found in the headers, can be found in r.Host. // but its alway necessary that the list of signed headers containing host in it. - val, ok := reqHeaders[http.CanonicalHeaderKey(header)] if !ok { // Golang http server strips off 'Expect' header, if the @@ -153,7 +157,6 @@ func extractSignedHeaders(signedHeaders []string, reqHeaders http.Header) (http. } extractedSignedHeaders[header] = val } - return extractedSignedHeaders, ErrNone } diff --git a/cmd/signature-v4_test.go b/cmd/signature-v4_test.go index 48763a991..c55520866 100644 --- a/cmd/signature-v4_test.go +++ b/cmd/signature-v4_test.go @@ -111,6 +111,8 @@ func TestDoesPresignedSignatureMatch(t *testing.T) { now := time.Now().UTC() credentialTemplate := "%s/%s/%s/s3/aws4_request" + region := serverConfig.GetRegion() + accessKeyID := serverConfig.GetCredential().AccessKeyID testCases := []struct { queryParams map[string]string headers map[string]string @@ -143,7 +145,7 @@ func TestDoesPresignedSignatureMatch(t *testing.T) { "X-Amz-Expires": "60", "X-Amz-Signature": "badsignature", "X-Amz-SignedHeaders": "host;x-amz-content-sha256;x-amz-date", - "X-Amz-Credential": fmt.Sprintf(credentialTemplate, serverConfig.GetCredential().AccessKeyID, now.Format(yyyymmdd), "us-west-1"), + "X-Amz-Credential": fmt.Sprintf(credentialTemplate, accessKeyID, now.Format(yyyymmdd), "us-west-1"), "X-Amz-Content-Sha256": "ThisIsNotThePayloadHash", }, region: "us-west-1", @@ -157,7 +159,7 @@ func TestDoesPresignedSignatureMatch(t *testing.T) { "X-Amz-Expires": "60", "X-Amz-Signature": "badsignature", "X-Amz-SignedHeaders": "host;x-amz-content-sha256;x-amz-date", - "X-Amz-Credential": fmt.Sprintf(credentialTemplate, serverConfig.GetCredential().AccessKeyID, now.Format(yyyymmdd), "us-west-1"), + "X-Amz-Credential": fmt.Sprintf(credentialTemplate, accessKeyID, now.Format(yyyymmdd), "us-west-1"), "X-Amz-Content-Sha256": payloadSHA256, }, region: "us-east-1", @@ -171,7 +173,7 @@ func TestDoesPresignedSignatureMatch(t *testing.T) { "X-Amz-Expires": "60", "X-Amz-Signature": "badsignature", "X-Amz-SignedHeaders": "host;x-amz-content-sha256;x-amz-date", - "X-Amz-Credential": fmt.Sprintf(credentialTemplate, serverConfig.GetCredential().AccessKeyID, now.Format(yyyymmdd), "us-west-1"), + "X-Amz-Credential": fmt.Sprintf(credentialTemplate, accessKeyID, now.Format(yyyymmdd), "us-west-1"), "X-Amz-Content-Sha256": payloadSHA256, }, region: "us-west-1", @@ -185,10 +187,10 @@ func TestDoesPresignedSignatureMatch(t *testing.T) { "X-Amz-Expires": "60", "X-Amz-Signature": "badsignature", "X-Amz-SignedHeaders": "x-amz-content-sha256;x-amz-date", - "X-Amz-Credential": fmt.Sprintf(credentialTemplate, serverConfig.GetCredential().AccessKeyID, now.Format(yyyymmdd), serverConfig.GetRegion()), + "X-Amz-Credential": fmt.Sprintf(credentialTemplate, accessKeyID, now.Format(yyyymmdd), region), "X-Amz-Content-Sha256": payloadSHA256, }, - region: serverConfig.GetRegion(), + region: region, expected: ErrUnsignedHeaders, }, // (6) Should give an expired request if it has expired. @@ -199,14 +201,14 @@ func TestDoesPresignedSignatureMatch(t *testing.T) { "X-Amz-Expires": "60", "X-Amz-Signature": "badsignature", "X-Amz-SignedHeaders": "host;x-amz-content-sha256;x-amz-date", - "X-Amz-Credential": fmt.Sprintf(credentialTemplate, serverConfig.GetCredential().AccessKeyID, now.Format(yyyymmdd), serverConfig.GetRegion()), + "X-Amz-Credential": fmt.Sprintf(credentialTemplate, accessKeyID, now.Format(yyyymmdd), region), "X-Amz-Content-Sha256": payloadSHA256, }, headers: map[string]string{ "X-Amz-Date": now.AddDate(0, 0, -2).Format(iso8601Format), "X-Amz-Content-Sha256": payloadSHA256, }, - region: serverConfig.GetRegion(), + region: region, expected: ErrExpiredPresignRequest, }, // (7) Should error if the signature is incorrect. @@ -217,14 +219,14 @@ func TestDoesPresignedSignatureMatch(t *testing.T) { "X-Amz-Expires": "60", "X-Amz-Signature": "badsignature", "X-Amz-SignedHeaders": "host;x-amz-content-sha256;x-amz-date", - "X-Amz-Credential": fmt.Sprintf(credentialTemplate, serverConfig.GetCredential().AccessKeyID, now.Format(yyyymmdd), serverConfig.GetRegion()), + "X-Amz-Credential": fmt.Sprintf(credentialTemplate, accessKeyID, now.Format(yyyymmdd), region), "X-Amz-Content-Sha256": payloadSHA256, }, headers: map[string]string{ "X-Amz-Date": now.Format(iso8601Format), "X-Amz-Content-Sha256": payloadSHA256, }, - region: serverConfig.GetRegion(), + region: region, expected: ErrSignatureDoesNotMatch, }, // (8) Should error if the request is not ready yet, ie X-Amz-Date is in the future. @@ -235,14 +237,14 @@ func TestDoesPresignedSignatureMatch(t *testing.T) { "X-Amz-Expires": "60", "X-Amz-Signature": "badsignature", "X-Amz-SignedHeaders": "host;x-amz-content-sha256;x-amz-date", - "X-Amz-Credential": fmt.Sprintf(credentialTemplate, serverConfig.GetCredential().AccessKeyID, now.Format(yyyymmdd), serverConfig.GetRegion()), + "X-Amz-Credential": fmt.Sprintf(credentialTemplate, accessKeyID, now.Format(yyyymmdd), region), "X-Amz-Content-Sha256": payloadSHA256, }, headers: map[string]string{ "X-Amz-Date": now.Format(iso8601Format), "X-Amz-Content-Sha256": payloadSHA256, }, - region: serverConfig.GetRegion(), + region: region, expected: ErrRequestNotReadyYet, }, // (9) Should not error with invalid region instead, call should proceed @@ -254,7 +256,7 @@ func TestDoesPresignedSignatureMatch(t *testing.T) { "X-Amz-Expires": "60", "X-Amz-Signature": "badsignature", "X-Amz-SignedHeaders": "host;x-amz-content-sha256;x-amz-date", - "X-Amz-Credential": fmt.Sprintf(credentialTemplate, serverConfig.GetCredential().AccessKeyID, now.Format(yyyymmdd), serverConfig.GetRegion()), + "X-Amz-Credential": fmt.Sprintf(credentialTemplate, accessKeyID, now.Format(yyyymmdd), region), "X-Amz-Content-Sha256": payloadSHA256, }, headers: map[string]string{ @@ -273,7 +275,7 @@ func TestDoesPresignedSignatureMatch(t *testing.T) { "X-Amz-Expires": "60", "X-Amz-Signature": "badsignature", "X-Amz-SignedHeaders": "host;x-amz-content-sha256;x-amz-date", - "X-Amz-Credential": fmt.Sprintf(credentialTemplate, serverConfig.GetCredential().AccessKeyID, now.Format(yyyymmdd), serverConfig.GetRegion()), + "X-Amz-Credential": fmt.Sprintf(credentialTemplate, accessKeyID, now.Format(yyyymmdd), region), "X-Amz-Content-Sha256": payloadSHA256, "response-content-type": "application/json", }, @@ -284,6 +286,24 @@ func TestDoesPresignedSignatureMatch(t *testing.T) { region: "", expected: ErrSignatureDoesNotMatch, }, + // (11) Should error with unsigned headers. + { + queryParams: map[string]string{ + "X-Amz-Algorithm": signV4Algorithm, + "X-Amz-Date": now.Format(iso8601Format), + "X-Amz-Expires": "60", + "X-Amz-Signature": "badsignature", + "X-Amz-SignedHeaders": "host;x-amz-content-sha256;x-amz-date", + "X-Amz-Credential": fmt.Sprintf(credentialTemplate, accessKeyID, now.Format(yyyymmdd), region), + "X-Amz-Content-Sha256": payloadSHA256, + "response-content-type": "application/json", + }, + headers: map[string]string{ + "X-Amz-Date": now.Format(iso8601Format), + }, + region: "", + expected: ErrUnsignedHeaders, + }, } // Run each test case individually. diff --git a/cmd/test-utils_test.go b/cmd/test-utils_test.go index 16bb8f359..d1a22277f 100644 --- a/cmd/test-utils_test.go +++ b/cmd/test-utils_test.go @@ -844,6 +844,45 @@ func queryEncode(v url.Values) string { return buf.String() } +// preSignV4 presign the request, in accordance with +// http://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html. +func preSignV4(req *http.Request, accessKeyID, secretAccessKey string, expires int64) error { + // Presign is not needed for anonymous credentials. + if accessKeyID == "" || secretAccessKey == "" { + return errors.New("Presign cannot be generated without access and secret keys") + } + + region := serverConfig.GetRegion() + date := time.Now().UTC() + credential := fmt.Sprintf("%s/%s", accessKeyID, getScope(date, region)) + + // Set URL query. + query := req.URL.Query() + query.Set("X-Amz-Algorithm", signV4Algorithm) + query.Set("X-Amz-Date", date.Format(iso8601Format)) + query.Set("X-Amz-Expires", strconv.FormatInt(expires, 10)) + query.Set("X-Amz-SignedHeaders", "host") + query.Set("X-Amz-Credential", credential) + query.Set("X-Amz-Content-Sha256", unsignedPayload) + + // Headers are empty, since "host" is the only header required to be signed for Presigned URLs. + var extractedSignedHeaders http.Header + + queryStr := strings.Replace(query.Encode(), "+", "%20", -1) + canonicalRequest := getCanonicalRequest(extractedSignedHeaders, unsignedPayload, queryStr, req.URL.Path, req.Method, req.Host) + stringToSign := getStringToSign(canonicalRequest, date, region) + signingKey := getSigningKey(secretAccessKey, date, region) + signature := getSignature(signingKey, stringToSign) + + req.URL.RawQuery = query.Encode() + + // Add signature header to RawQuery. + req.URL.RawQuery += "&X-Amz-Signature=" + signature + + // Construct the final presigned URL. + return nil +} + // preSignV2 - presign the request in following style. // https://${S3_BUCKET}.s3.amazonaws.com/${S3_OBJECT}?AWSAccessKeyId=${S3_ACCESS_KEY}&Expires=${TIMESTAMP}&Signature=${SIGNATURE}. func preSignV2(req *http.Request, accessKeyID, secretAccessKey string, expires int64) error { diff --git a/cmd/web-handlers.go b/cmd/web-handlers.go index 9b1f616ba..a86eddf75 100644 --- a/cmd/web-handlers.go +++ b/cmd/web-handlers.go @@ -796,7 +796,7 @@ func presignedGet(host, bucket, object string) string { secretKey := cred.SecretAccessKey date := time.Now().UTC() - dateStr := date.Format("20060102T150405Z") + dateStr := date.Format(iso8601Format) credential := fmt.Sprintf("%s/%s", accessKey, getScope(date, region)) query := strings.Join([]string{