From 17e49a9ed2e5819480110f9fa30c5c526614833d Mon Sep 17 00:00:00 2001 From: Karthic Rao Date: Thu, 13 Oct 2016 21:55:56 +0530 Subject: [PATCH] signature-v2 fix. (#2918) - Return errors similar to V4 Sign processsing. - Return ErrMissing fields when Auth Header fields are missing. - Return InvalidAccessID when accessID doesn't match. * tests: Adding V2 signature tests for bucket handler API's. --- cmd/bucket-handlers_test.go | 299 ++++++++++++++++++++++++++++++------ cmd/signature-v2.go | 41 ++++- cmd/signature-v2_test.go | 76 +++++++++ 3 files changed, 367 insertions(+), 49 deletions(-) diff --git a/cmd/bucket-handlers_test.go b/cmd/bucket-handlers_test.go index 9f42507c1..3febf730e 100644 --- a/cmd/bucket-handlers_test.go +++ b/cmd/bucket-handlers_test.go @@ -44,34 +44,39 @@ func testGetBucketLocationHandler(obj ObjectLayer, instanceType, bucketName stri errorResponse APIErrorResponse shouldPass bool }{ + // Test case - 1. // Tests for authenticated request and proper response. { - bucketName, - credentials.AccessKeyID, - credentials.SecretAccessKey, - http.StatusOK, - []byte(` + bucketName: bucketName, + accessKey: credentials.AccessKeyID, + secretKey: credentials.SecretAccessKey, + expectedRespStatus: http.StatusOK, + locationResponse: []byte(` `), - APIErrorResponse{}, - true, + errorResponse: APIErrorResponse{}, + shouldPass: true, }, - // Tests for anonymous requests. + // Test case - 2. + // Tests for signature mismatch error. { - bucketName, - "", - "", - http.StatusForbidden, - []byte(""), - APIErrorResponse{ + bucketName: bucketName, + accessKey: "abcd", + secretKey: "abcd", + expectedRespStatus: http.StatusForbidden, + locationResponse: []byte(""), + errorResponse: APIErrorResponse{ Resource: "/" + bucketName + "/", - Code: "AccessDenied", - Message: "Access Denied.", + Code: "InvalidAccessKeyID", + Message: "The access key ID you provided does not exist in our records.", }, - false, + shouldPass: false, }, } for i, testCase := range testCases { + if i != 1 { + continue + } // initialize httptest Recorder, this records any mutations to response writer inside the handler. rec := httptest.NewRecorder() // construct HTTP request for Get bucket location. @@ -79,7 +84,7 @@ func testGetBucketLocationHandler(obj ObjectLayer, instanceType, bucketName stri if err != nil { t.Fatalf("Test %d: %s: Failed to create HTTP request for GetBucketLocationHandler: %v", i+1, instanceType, err) } - // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic ofthe handler. + // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. // Call the ServeHTTP to execute the handler. apiRouter.ServeHTTP(rec, req) if rec.Code != testCase.expectedRespStatus { @@ -102,6 +107,38 @@ func testGetBucketLocationHandler(obj ObjectLayer, instanceType, bucketName stri if errorResponse.Code != testCase.errorResponse.Code { t.Errorf("Test %d: %s: Expected the error code to be `%s`, but instead found `%s`", i+1, instanceType, testCase.errorResponse.Code, errorResponse.Code) } + + // Verify response of the V2 signed HTTP request. + // initialize HTTP NewRecorder, this records any mutations to response writer inside the handler. + recV2 := httptest.NewRecorder() + // construct HTTP request for PUT bucket policy endpoint. + reqV2, err := newTestSignedRequestV2("GET", getBucketLocationURL("", testCase.bucketName), 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) + } + // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. + // Call the ServeHTTP to execute the handler. + apiRouter.ServeHTTP(recV2, reqV2) + if recV2.Code != testCase.expectedRespStatus { + t.Errorf("Test %d: %s: Expected the response status to be `%d`, but instead found `%d`", i+1, instanceType, testCase.expectedRespStatus, recV2.Code) + } + + errorResponse = APIErrorResponse{} + err = xml.Unmarshal(recV2.Body.Bytes(), &errorResponse) + if err != nil && !testCase.shouldPass { + t.Fatalf("Test %d: %s: Unable to marshal response body %s", i+1, instanceType, string(recV2.Body.Bytes())) + } + if errorResponse.Resource != testCase.errorResponse.Resource { + t.Errorf("Test %d: %s: Expected the error resource to be `%s`, but instead found `%s`", i+1, instanceType, testCase.errorResponse.Resource, errorResponse.Resource) + } + if errorResponse.Message != testCase.errorResponse.Message { + t.Errorf("Test %d: %s: Expected the error message to be `%s`, but instead found `%s`", i+1, instanceType, testCase.errorResponse.Message, errorResponse.Message) + } + if errorResponse.Code != testCase.errorResponse.Code { + t.Errorf("Test %d: %s: Expected the error code to be `%s`, but instead found `%s`", i+1, instanceType, testCase.errorResponse.Code, errorResponse.Code) + } + } // Test for Anonymous/unsigned http request. @@ -149,6 +186,7 @@ func testHeadBucketHandler(obj ObjectLayer, instanceType, bucketName string, api // expected Response. expectedRespStatus int }{ + // Test case - 1. // Bucket exists. { bucketName: bucketName, @@ -156,6 +194,7 @@ func testHeadBucketHandler(obj ObjectLayer, instanceType, bucketName string, api secretKey: credentials.SecretAccessKey, expectedRespStatus: http.StatusOK, }, + // Test case - 2. // Non-existent bucket name. { bucketName: "2333", @@ -163,11 +202,13 @@ func testHeadBucketHandler(obj ObjectLayer, instanceType, bucketName string, api secretKey: credentials.SecretAccessKey, expectedRespStatus: http.StatusNotFound, }, - // Un-authenticated request. + // Test case - 3. + // Testing for signature mismatch error. + // setting invalid acess and secret key. { bucketName: bucketName, - accessKey: "", - secretKey: "", + accessKey: "abcd", + secretKey: "abcd", expectedRespStatus: http.StatusForbidden, }, } @@ -180,12 +221,29 @@ func testHeadBucketHandler(obj ObjectLayer, instanceType, bucketName string, api if err != nil { t.Fatalf("Test %d: %s: Failed to create HTTP request for HeadBucketHandler: %v", i+1, instanceType, err) } - // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic ofthe handler. + // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. // Call the ServeHTTP to execute the handler. apiRouter.ServeHTTP(rec, req) if rec.Code != testCase.expectedRespStatus { t.Errorf("Test %d: %s: Expected the response status to be `%d`, but instead found `%d`", i+1, instanceType, testCase.expectedRespStatus, rec.Code) } + + // Verify response the V2 signed HTTP request. + // initialize HTTP NewRecorder, this records any mutations to response writer inside the handler. + recV2 := httptest.NewRecorder() + // construct HTTP request for PUT bucket policy endpoint. + reqV2, err := newTestSignedRequestV2("HEAD", getHEADBucketURL("", testCase.bucketName), 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) + } + // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. + // Call the ServeHTTP to execute the handler. + apiRouter.ServeHTTP(recV2, reqV2) + if recV2.Code != testCase.expectedRespStatus { + t.Errorf("Test %d: %s: Expected the response status to be `%d`, but instead found `%d`", i+1, instanceType, testCase.expectedRespStatus, recV2.Code) + } + } // Test for Anonymous/unsigned http request. @@ -237,25 +295,138 @@ func testListMultipartUploadsHandler(obj ObjectLayer, instanceType, bucketName s uploadIDMarker string delimiter string maxUploads string + accessKey string + secretKey string expectedRespStatus int shouldPass bool }{ - // 1 - invalid bucket name. - {".test", "", "", "", "", "0", http.StatusBadRequest, false}, - // 2 - bucket not found. - {"volatile-bucket-1", "", "", "", "", "0", http.StatusNotFound, false}, - // 3 - invalid delimiter. - {bucketName, "", "", "", "-", "0", http.StatusNotImplemented, false}, - // 4 - invalid prefix and marker combination. - {bucketName, "asia", "europe-object", "", "", "0", http.StatusNotImplemented, false}, - // 5 - invalid upload id and marker combination. - {bucketName, "asia", "asia/europe/", "abc", "", "0", http.StatusNotImplemented, false}, - // 6 - invalid max uploads. - {bucketName, "", "", "", "", "-1", http.StatusBadRequest, false}, - // 7 - good case delimiter. - {bucketName, "", "", "", "/", "100", http.StatusOK, true}, - // 8 - good case without delimiter. - {bucketName, "", "", "", "", "100", http.StatusOK, true}, + // Test case - 1. + // Setting invalid bucket name. + { + bucket: ".test", + prefix: "", + keyMarker: "", + uploadIDMarker: "", + delimiter: "", + maxUploads: "0", + accessKey: credentials.AccessKeyID, + secretKey: credentials.SecretAccessKey, + expectedRespStatus: http.StatusBadRequest, + shouldPass: false, + }, + // Test case - 2. + // Setting a non-existent bucket. + { + bucket: "volatile-bucket-1", + prefix: "", + keyMarker: "", + uploadIDMarker: "", + delimiter: "", + maxUploads: "0", + accessKey: credentials.AccessKeyID, + secretKey: credentials.SecretAccessKey, + expectedRespStatus: http.StatusNotFound, + shouldPass: false, + }, + // Test case -3. + // Setting invalid delimiter, expecting the HTTP response status to be http.StatusNotImplemented. + { + bucket: bucketName, + prefix: "", + keyMarker: "", + uploadIDMarker: "", + delimiter: "-", + maxUploads: "0", + accessKey: credentials.AccessKeyID, + secretKey: credentials.SecretAccessKey, + expectedRespStatus: http.StatusNotImplemented, + shouldPass: false, + }, + // Test case - 4. + // Setting Invalid prefix and marker combination. + { + bucket: bucketName, + prefix: "asia", + keyMarker: "europe-object", + uploadIDMarker: "", + delimiter: "", + maxUploads: "0", + accessKey: credentials.AccessKeyID, + secretKey: credentials.SecretAccessKey, + expectedRespStatus: http.StatusNotImplemented, + shouldPass: false, + }, + // Test case - 5. + // Invalid upload id and marker combination. + { + bucket: bucketName, + prefix: "asia", + keyMarker: "asia/europe/", + uploadIDMarker: "abc", + delimiter: "", + maxUploads: "0", + accessKey: credentials.AccessKeyID, + secretKey: credentials.SecretAccessKey, + expectedRespStatus: http.StatusNotImplemented, + shouldPass: false, + }, + // Test case - 6. + // Setting a negative value to max-uploads paramater, should result in http.StatusBadRequest. + { + bucket: bucketName, + prefix: "", + keyMarker: "", + uploadIDMarker: "", + delimiter: "", + maxUploads: "-1", + accessKey: credentials.AccessKeyID, + secretKey: credentials.SecretAccessKey, + expectedRespStatus: http.StatusBadRequest, + shouldPass: false, + }, + // Test case - 7. + // Case with right set of parameters, + // should result in success 200OK. + { + bucket: bucketName, + prefix: "", + keyMarker: "", + uploadIDMarker: "", + delimiter: "/", + maxUploads: "100", + accessKey: credentials.AccessKeyID, + secretKey: credentials.SecretAccessKey, + expectedRespStatus: http.StatusOK, + shouldPass: true, + }, + // Test case - 8. + // Good case without delimiter. + { + bucket: bucketName, + prefix: "", + keyMarker: "", + uploadIDMarker: "", + delimiter: "", + maxUploads: "100", + accessKey: credentials.AccessKeyID, + secretKey: credentials.SecretAccessKey, + expectedRespStatus: http.StatusOK, + shouldPass: true, + }, + // Test case - 9. + // Setting Invalid AccessKey and SecretKey to induce and verify Signature Mismatch error. + { + bucket: bucketName, + prefix: "", + keyMarker: "", + uploadIDMarker: "", + delimiter: "", + maxUploads: "100", + accessKey: "abcd", + secretKey: "abcd", + expectedRespStatus: http.StatusForbidden, + shouldPass: true, + }, } for i, testCase := range testCases { @@ -264,16 +435,34 @@ func testListMultipartUploadsHandler(obj ObjectLayer, instanceType, bucketName s // construct HTTP request for List multipart uploads endpoint. u := getListMultipartUploadsURLWithParams("", testCase.bucket, testCase.prefix, testCase.keyMarker, testCase.uploadIDMarker, testCase.delimiter, testCase.maxUploads) - req, gerr := newTestSignedRequestV4("GET", u, 0, nil, credentials.AccessKeyID, credentials.SecretAccessKey) + req, gerr := newTestSignedRequestV4("GET", u, 0, nil, testCase.accessKey, testCase.secretKey) if gerr != nil { t.Fatalf("Test %d: %s: Failed to create HTTP request for ListMultipartUploadsHandler: %v", i+1, instanceType, gerr) } - // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic ofthe handler. + // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. // Call the ServeHTTP to execute the handler. apiRouter.ServeHTTP(rec, req) if rec.Code != testCase.expectedRespStatus { t.Errorf("Test %d: %s: Expected the response status to be `%d`, but instead found `%d`", i+1, instanceType, testCase.expectedRespStatus, rec.Code) } + + // Verify response the V2 signed HTTP request. + // initialize HTTP NewRecorder, this records any mutations to response writer inside the handler. + recV2 := httptest.NewRecorder() + // construct HTTP request for PUT bucket policy endpoint. + + // 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) + } + // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. + // Call the ServeHTTP to execute the handler. + apiRouter.ServeHTTP(recV2, reqV2) + if recV2.Code != testCase.expectedRespStatus { + t.Errorf("Test %d: %s: Expected the response status to be `%d`, but instead found `%d`", i+1, instanceType, testCase.expectedRespStatus, recV2.Code) + } } // initialize HTTP NewRecorder, this records any mutations to response writer inside the handler. @@ -285,7 +474,7 @@ func testListMultipartUploadsHandler(obj ObjectLayer, instanceType, bucketName s if err != nil { t.Fatalf("Test %s: Failed to create HTTP request for ListMultipartUploadsHandler: %v", instanceType, err) } - // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic ofthe handler. + // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. // Call the ServeHTTP to execute the handler. apiRouter.ServeHTTP(rec, req) if rec.Code != http.StatusForbidden { @@ -340,6 +529,7 @@ func testListBucketsHandler(obj ObjectLayer, instanceType, bucketName string, ap secretKey string expectedRespStatus int }{ + // Test case - 1. // Validate a good case request succeeds. { bucketName: bucketName, @@ -347,11 +537,12 @@ func testListBucketsHandler(obj ObjectLayer, instanceType, bucketName string, ap secretKey: credentials.SecretAccessKey, expectedRespStatus: http.StatusOK, }, - // Validate a bad case request fails with http.StatusForbidden. + // Test case - 2. + // Test case with invalid accessKey to produce and validate Signature MisMatch error. { bucketName: bucketName, - accessKey: "", - secretKey: "", + accessKey: "abcd", + secretKey: "abcd", expectedRespStatus: http.StatusForbidden, }, } @@ -363,12 +554,30 @@ func testListBucketsHandler(obj ObjectLayer, instanceType, bucketName string, ap if lerr != nil { t.Fatalf("Test %d: %s: Failed to create HTTP request for ListBucketsHandler: %v", i+1, instanceType, lerr) } - // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic ofthe handler. + // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. // Call the ServeHTTP to execute the handler. apiRouter.ServeHTTP(rec, req) if rec.Code != testCase.expectedRespStatus { t.Errorf("Test %d: %s: Expected the response status to be `%d`, but instead found `%d`", i+1, instanceType, testCase.expectedRespStatus, rec.Code) } + + // Verify response of the V2 signed HTTP request. + // initialize HTTP NewRecorder, this records any mutations to response writer inside the handler. + recV2 := httptest.NewRecorder() + // construct HTTP request for PUT bucket policy endpoint. + + // verify response for V2 signed HTTP request. + reqV2, err := newTestSignedRequestV2("GET", getListBucketURL(""), 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) + } + // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. + // Call the ServeHTTP to execute the handler. + apiRouter.ServeHTTP(recV2, reqV2) + if recV2.Code != testCase.expectedRespStatus { + t.Errorf("Test %d: %s: Expected the response status to be `%d`, but instead found `%d`", i+1, instanceType, testCase.expectedRespStatus, recV2.Code) + } } // Test for Anonymous/unsigned http request. diff --git a/cmd/signature-v2.go b/cmd/signature-v2.go index 14dee907e..8957c117e 100644 --- a/cmd/signature-v2.go +++ b/cmd/signature-v2.go @@ -140,11 +140,44 @@ func doesPresignV2SignatureMatch(r *http.Request) APIErrorCode { // doesSignV2Match - Verify authorization header with calculated header in accordance with // - http://docs.aws.amazon.com/AmazonS3/latest/dev/auth-request-sig-v2.html // returns true if matches, false otherwise. if error is not nil then it is always false -func doesSignV2Match(r *http.Request) APIErrorCode { - gotAuth := r.Header.Get("Authorization") - if gotAuth == "" { + +func validateV2AuthHeader(v2Auth string) APIErrorCode { + if v2Auth == "" { return ErrAuthHeaderEmpty } + // Verify if the header algorithm is supported or not. + if !strings.HasPrefix(v2Auth, signV2Algorithm) { + return ErrSignatureVersionNotSupported + } + + // below is V2 Signed Auth header format, splitting on `space` (after the `AWS` string). + // Authorization = "AWS" + " " + AWSAccessKeyId + ":" + Signature + authFields := strings.Split(v2Auth, " ") + if len(authFields) != 2 { + return ErrMissingFields + } + + // Then will be splitting on ":", this will seprate `AWSAccessKeyId` and `Signature` string. + keySignFields := strings.Split(strings.TrimSpace(authFields[1]), ":") + if len(keySignFields) != 2 { + return ErrMissingFields + } + + // Access credentials. + cred := serverConfig.GetCredential() + if keySignFields[0] != cred.AccessKeyID { + return ErrInvalidAccessKeyID + } + + return ErrNone +} + +func doesSignV2Match(r *http.Request) APIErrorCode { + v2Auth := r.Header.Get("Authorization") + + if apiError := validateV2AuthHeader(v2Auth); apiError != ErrNone { + return apiError + } // 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?) @@ -158,7 +191,7 @@ func doesSignV2Match(r *http.Request) APIErrorCode { } expectedAuth := signatureV2(r.Method, encodedResource, encodedQuery, r.Header) - if gotAuth != expectedAuth { + if v2Auth != expectedAuth { return ErrSignatureDoesNotMatch } diff --git a/cmd/signature-v2_test.go b/cmd/signature-v2_test.go index 9813187db..64966507f 100644 --- a/cmd/signature-v2_test.go +++ b/cmd/signature-v2_test.go @@ -104,3 +104,79 @@ func TestDoesPresignedV2SignatureMatch(t *testing.T) { } } } + +// TestValidateV2AuthHeader - Tests validate the logic of V2 Authorization header validator. +func TestValidateV2AuthHeader(t *testing.T) { + // Initialize server config. + if err := initConfig(); err != nil { + t.Fatal(err) + } + + // Save config. + if err := serverConfig.Save(); err != nil { + t.Fatal(err) + } + accessID := serverConfig.GetCredential().AccessKeyID + + testCases := []struct { + authString string + expectedError APIErrorCode + }{ + // Test case - 1. + // Case with empty V2AuthString. + { + + authString: "", + expectedError: ErrAuthHeaderEmpty, + }, + // Test case - 2. + // Test case with `signV2Algorithm` ("AWS") not being the prefix. + { + + authString: "NoV2Prefix", + expectedError: ErrSignatureVersionNotSupported, + }, + // Test case - 3. + // Test case with missing parts in the Auth string. + // below is the correct format of V2 Authorization header. + // Authorization = "AWS" + " " + AWSAccessKeyId + ":" + Signature + { + + authString: signV2Algorithm, + expectedError: ErrMissingFields, + }, + // Test case - 4. + // Test case with signature part missing. + { + + authString: fmt.Sprintf("%s %s", signV2Algorithm, accessID), + expectedError: ErrMissingFields, + }, + // Test case - 5. + // Test case with wrong accessID. + { + + authString: fmt.Sprintf("%s %s:%s", signV2Algorithm, "InvalidAccessID", "signature"), + expectedError: ErrInvalidAccessKeyID, + }, + // Test case - 6. + // Case with right accessID and format. + { + + authString: fmt.Sprintf("%s %s:%s", signV2Algorithm, accessID, "signature"), + expectedError: ErrNone, + }, + } + + for i, testCase := range testCases { + t.Run(fmt.Sprintf("Case %d AuthStr \"%s\".", i+1, testCase.authString), func(t *testing.T) { + + actualErrCode := validateV2AuthHeader(testCase.authString) + + if testCase.expectedError != actualErrCode { + t.Errorf("Expected the error code to be %v, got %v.", testCase.expectedError, actualErrCode) + } + }) + } + +}