Merge pull request #427 from moorereason/feature/empty-payload-signature

Warn on failed validate of empty payload signature

internal/hook/hook.go

@@ -73,6 +73,8 @@ func IsParameterNodeError(err error) bool {
 type SignatureError struct {
 	Signature  string
 	Signatures []string
+
+	emptyPayload bool
 }
 
 func (e *SignatureError) Error() string {
@@ -80,11 +82,16 @@ func (e *SignatureError) Error() string {
 		return "<nil>"
 	}
 
-	if e.Signatures != nil {
+	var empty string
-		return fmt.Sprintf("invalid payload signatures %s", e.Signatures)
+	if e.emptyPayload {
+		empty = " on empty payload"
 	}
 
-	return fmt.Sprintf("invalid payload signature %s", e.Signature)
+	if e.Signatures != nil {
+		return fmt.Sprintf("invalid payload signatures %s%s", e.Signatures, empty)
+	}
+
+	return fmt.Sprintf("invalid payload signature %s%s", e.Signature, empty)
 }
 
 // ArgumentError describes an invalid argument passed to Hook.
@@ -160,21 +167,24 @@ func ValidateMAC(payload []byte, mac hash.Hash, signatures []string) (string, er
 		return "", err
 	}
 
-	expectedMAC := hex.EncodeToString(mac.Sum(nil))
+	actualMAC := hex.EncodeToString(mac.Sum(nil))
 
 	for _, signature := range signatures {
-		if hmac.Equal([]byte(signature), []byte(expectedMAC)) {
+		if hmac.Equal([]byte(signature), []byte(actualMAC)) {
-			return expectedMAC, err
+			return actualMAC, err
 		}
 	}
 
-	return expectedMAC, &SignatureError{
+	e := &SignatureError{Signatures: signatures}
-		Signatures: signatures,
+	if len(payload) == 0 {
+		e.emptyPayload = true
 	}
+
+	return actualMAC, e
 }
 
 // CheckPayloadSignature calculates and verifies SHA1 signature of the given payload
-func CheckPayloadSignature(payload []byte, secret string, signature string) (string, error) {
+func CheckPayloadSignature(payload []byte, secret, signature string) (string, error) {
 	if secret == "" {
 		return "", errors.New("signature validation secret can not be empty")
 	}
@@ -187,7 +197,7 @@ func CheckPayloadSignature(payload []byte, secret string, signature string) (str
 }
 
 // CheckPayloadSignature256 calculates and verifies SHA256 signature of the given payload
-func CheckPayloadSignature256(payload []byte, secret string, signature string) (string, error) {
+func CheckPayloadSignature256(payload []byte, secret, signature string) (string, error) {
 	if secret == "" {
 		return "", errors.New("signature validation secret can not be empty")
 	}
@@ -200,7 +210,7 @@ func CheckPayloadSignature256(payload []byte, secret string, signature string) (
 }
 
 // CheckPayloadSignature512 calculates and verifies SHA512 signature of the given payload
-func CheckPayloadSignature512(payload []byte, secret string, signature string) (string, error) {
+func CheckPayloadSignature512(payload []byte, secret, signature string) (string, error) {
 	if secret == "" {
 		return "", errors.New("signature validation secret can not be empty")
 	}
@@ -254,7 +264,7 @@ func CheckScalrSignature(headers map[string]interface{}, body []byte, signingKey
 
 // CheckIPWhitelist makes sure the provided remote address (of the form IP:port) falls within the provided IP range
 // (in CIDR form or a single IP address).
-func CheckIPWhitelist(remoteAddr string, ipRange string) (bool, error) {
+func CheckIPWhitelist(remoteAddr, ipRange string) (bool, error) {
 	// Extract IP address from remote address.
 
 	// IPv6 addresses will likely be surrounded by [].
@@ -293,7 +303,7 @@ func CheckIPWhitelist(remoteAddr string, ipRange string) (bool, error) {
 // ReplaceParameter replaces parameter value with the passed value in the passed map
 // (please note you should pass pointer to the map, because we're modifying it)
 // based on the passed string
-func ReplaceParameter(s string, params interface{}, value interface{}) bool {
+func ReplaceParameter(s string, params, value interface{}) bool {
 	if params == nil {
 		return false
 	}

internal/hook/hook_test.go

@@ -49,12 +49,14 @@ var checkPayloadSignatureTests = []struct {
 	{[]byte(`{"a": "z"}`), "secret", "b17e04cbb22afa8ffbff8796fc1894ed27badd9e", "b17e04cbb22afa8ffbff8796fc1894ed27badd9e", true},
 	{[]byte(`{"a": "z"}`), "secret", "sha1=b17e04cbb22afa8ffbff8796fc1894ed27badd9e", "b17e04cbb22afa8ffbff8796fc1894ed27badd9e", true},
 	{[]byte(`{"a": "z"}`), "secret", "sha1=XXXe04cbb22afa8ffbff8796fc1894ed27badd9e,sha1=b17e04cbb22afa8ffbff8796fc1894ed27badd9e", "b17e04cbb22afa8ffbff8796fc1894ed27badd9e", true},
+	{[]byte(``), "secret", "25af6174a0fcecc4d346680a72b7ce644b9a88e8", "25af6174a0fcecc4d346680a72b7ce644b9a88e8", true},
 	// failures
 	{[]byte(`{"a": "z"}`), "secret", "XXXe04cbb22afa8ffbff8796fc1894ed27badd9e", "b17e04cbb22afa8ffbff8796fc1894ed27badd9e", false},
 	{[]byte(`{"a": "z"}`), "secret", "sha1=XXXe04cbb22afa8ffbff8796fc1894ed27badd9e", "b17e04cbb22afa8ffbff8796fc1894ed27badd9e", false},
 	{[]byte(`{"a": "z"}`), "secret", "sha1=XXXe04cbb22afa8ffbff8796fc1894ed27badd9e,sha1=XXXe04cbb22afa8ffbff8796fc1894ed27badd9e", "b17e04cbb22afa8ffbff8796fc1894ed27badd9e", false},
 	{[]byte(`{"a": "z"}`), "secreX", "b17e04cbb22afa8ffbff8796fc1894ed27badd9e", "900225703e9342328db7307692736e2f7cc7b36e", false},
 	{[]byte(`{"a": "z"}`), "", "b17e04cbb22afa8ffbff8796fc1894ed27badd9e", "", false},
+	{[]byte(``), "secret", "XXXf6174a0fcecc4d346680a72b7ce644b9a88e8", "25af6174a0fcecc4d346680a72b7ce644b9a88e8", false},
 }
 
 func TestCheckPayloadSignature(t *testing.T) {
@@ -80,11 +82,13 @@ var checkPayloadSignature256Tests = []struct {
 	{[]byte(`{"a": "z"}`), "secret", "f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", "f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", true},
 	{[]byte(`{"a": "z"}`), "secret", "sha256=f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", "f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", true},
 	{[]byte(`{"a": "z"}`), "secret", "sha256=XXX7af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89,sha256=f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", "f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", true},
+	{[]byte(``), "secret", "f9e66e179b6747ae54108f82f8ade8b3c25d76fd30afde6c395822c530196169", "f9e66e179b6747ae54108f82f8ade8b3c25d76fd30afde6c395822c530196169", true},
 	// failures
 	{[]byte(`{"a": "z"}`), "secret", "XXX7af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", "f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", false},
 	{[]byte(`{"a": "z"}`), "secret", "sha256=XXX7af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", "f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", false},
 	{[]byte(`{"a": "z"}`), "secret", "sha256=XXX7af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89,sha256=XXX7af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", "f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", false},
 	{[]byte(`{"a": "z"}`), "", "XXX7af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", "", false},
+	{[]byte(``), "secret", "XXX66e179b6747ae54108f82f8ade8b3c25d76fd30afde6c395822c530196169", "f9e66e179b6747ae54108f82f8ade8b3c25d76fd30afde6c395822c530196169", false},
 }
 
 func TestCheckPayloadSignature256(t *testing.T) {
@@ -109,9 +113,11 @@ var checkPayloadSignature512Tests = []struct {
 }{
 	{[]byte(`{"a": "z"}`), "secret", "4ab17cc8ec668ead8bf498f87f8f32848c04d5ca3c9bcfcd3db9363f0deb44e580b329502a7fdff633d4d8fca301cc5c94a55a2fec458c675fb0ff2655898324", "4ab17cc8ec668ead8bf498f87f8f32848c04d5ca3c9bcfcd3db9363f0deb44e580b329502a7fdff633d4d8fca301cc5c94a55a2fec458c675fb0ff2655898324", true},
 	{[]byte(`{"a": "z"}`), "secret", "sha512=4ab17cc8ec668ead8bf498f87f8f32848c04d5ca3c9bcfcd3db9363f0deb44e580b329502a7fdff633d4d8fca301cc5c94a55a2fec458c675fb0ff2655898324", "4ab17cc8ec668ead8bf498f87f8f32848c04d5ca3c9bcfcd3db9363f0deb44e580b329502a7fdff633d4d8fca301cc5c94a55a2fec458c675fb0ff2655898324", true},
+	{[]byte(``), "secret", "b0e9650c5faf9cd8ae02276671545424104589b3656731ec193b25d01b07561c27637c2d4d68389d6cf5007a8632c26ec89ba80a01c77a6cdd389ec28db43901", "b0e9650c5faf9cd8ae02276671545424104589b3656731ec193b25d01b07561c27637c2d4d68389d6cf5007a8632c26ec89ba80a01c77a6cdd389ec28db43901", true},
 	// failures
 	{[]byte(`{"a": "z"}`), "secret", "74a0081f5b5988f4f3e8b8dd34dadc6291611f2e6260635a7e1535f8e95edb97ff520ba8b152e8ca5760ac42639854f3242e29efc81be73a8bf52d474d31ffea", "4ab17cc8ec668ead8bf498f87f8f32848c04d5ca3c9bcfcd3db9363f0deb44e580b329502a7fdff633d4d8fca301cc5c94a55a2fec458c675fb0ff2655898324", false},
 	{[]byte(`{"a": "z"}`), "", "74a0081f5b5988f4f3e8b8dd34dadc6291611f2e6260635a7e1535f8e95edb97ff520ba8b152e8ca5760ac42639854f3242e29efc81be73a8bf52d474d31ffea", "", false},
+	{[]byte(``), "secret", "XXX9650c5faf9cd8ae02276671545424104589b3656731ec193b25d01b07561c27637c2d4d68389d6cf5007a8632c26ec89ba80a01c77a6cdd389ec28db43901", "b0e9650c5faf9cd8ae02276671545424104589b3656731ec193b25d01b07561c27637c2d4d68389d6cf5007a8632c26ec89ba80a01c77a6cdd389ec28db43901", false},
 }
 
 func TestCheckPayloadSignature512(t *testing.T) {
@@ -502,7 +508,8 @@ var andRuleTests = []struct {
 			{Match: &MatchRule{"value", "", "", "z", Argument{"header", "a", "", false}, ""}},
 			{Match: &MatchRule{"value", "", "", "y", Argument{"header", "b", "", false}, ""}},
 		},
-		&map[string]interface{}{"A": "z", "B": "y"}, nil, nil, []byte{},
+		&map[string]interface{}{"A": "z", "B": "y"}, nil, nil,
+		[]byte{},
 		true, false,
 	},
 	{
@@ -511,7 +518,8 @@ var andRuleTests = []struct {
 			{Match: &MatchRule{"value", "", "", "z", Argument{"header", "a", "", false}, ""}},
 			{Match: &MatchRule{"value", "", "", "y", Argument{"header", "b", "", false}, ""}},
 		},
-		&map[string]interface{}{"A": "z", "B": "Y"}, nil, nil, []byte{},
+		&map[string]interface{}{"A": "z", "B": "Y"}, nil, nil,
+		[]byte{},
 		false, false,
 	},
 	// Complex test to cover Rules.Evaluate
@@ -537,7 +545,8 @@ var andRuleTests = []struct {
 				},
 			},
 		},
-		&map[string]interface{}{"A": "z", "B": "y", "C": "x", "D": "w", "E": "X", "F": "X"}, nil, nil, []byte{},
+		&map[string]interface{}{"A": "z", "B": "y", "C": "x", "D": "w", "E": "X", "F": "X"}, nil, nil,
+		[]byte{},
 		true, false,
 	},
 	{"empty rule", AndRule{{}}, nil, nil, nil, nil, false, false},
@@ -573,7 +582,8 @@ var orRuleTests = []struct {
 			{Match: &MatchRule{"value", "", "", "z", Argument{"header", "a", "", false}, ""}},
 			{Match: &MatchRule{"value", "", "", "y", Argument{"header", "b", "", false}, ""}},
 		},
-		&map[string]interface{}{"A": "z", "B": "X"}, nil, nil, []byte{},
+		&map[string]interface{}{"A": "z", "B": "X"}, nil, nil,
+		[]byte{},
 		true, false,
 	},
 	{
@@ -582,7 +592,8 @@ var orRuleTests = []struct {
 			{Match: &MatchRule{"value", "", "", "z", Argument{"header", "a", "", false}, ""}},
 			{Match: &MatchRule{"value", "", "", "y", Argument{"header", "b", "", false}, ""}},
 		},
-		&map[string]interface{}{"A": "X", "B": "y"}, nil, nil, []byte{},
+		&map[string]interface{}{"A": "X", "B": "y"}, nil, nil,
+		[]byte{},
 		true, false,
 	},
 	{
@@ -591,7 +602,8 @@ var orRuleTests = []struct {
 			{Match: &MatchRule{"value", "", "", "z", Argument{"header", "a", "", false}, ""}},
 			{Match: &MatchRule{"value", "", "", "y", Argument{"header", "b", "", false}, ""}},
 		},
-		&map[string]interface{}{"A": "Z", "B": "Y"}, nil, nil, []byte{},
+		&map[string]interface{}{"A": "Z", "B": "Y"}, nil, nil,
+		[]byte{},
 		false, false,
 	},
 	// failures
@@ -600,7 +612,8 @@ var orRuleTests = []struct {
 		OrRule{
 			{Match: &MatchRule{"value", "", "", "z", Argument{"header", "a", "", false}, ""}},
 		},
-		&map[string]interface{}{"Y": "Z"}, nil, nil, []byte{},
+		&map[string]interface{}{"Y": "Z"}, nil, nil,
+		[]byte{},
 		false, true,
 	},
 }

test/hooks.json.tmpl

@@ -259,5 +259,29 @@
         "name": "passed"
       }
     ],
+  },
+  {
+    "id": "empty-payload-signature",
+    "execute-command": "{{ .Hookecho }}",
+    "command-working-directory": "/",
+    "include-command-output-in-response": true,
+    "trigger-rule":
+    {
+      "and":
+      [
+        {
+          "match":
+          {
+            "type": "payload-hash-sha1",
+            "secret": "mysecret",
+            "parameter":
+            {
+              "source": "header",
+              "name": "X-Hub-Signature"
+            }
+          }
+        }
+      ]
+    }
   }
 ]

test/hooks.yaml.tmpl

@@ -150,3 +150,16 @@
 - id: warn-on-space
   execute-command: '{{ .Hookecho }} foo'
   include-command-output-in-response: true
+
+- id: empty-payload-signature
+  include-command-output-in-response: true
+  execute-command: '{{ .Hookecho }}'
+  command-working-directory: /
+  trigger-rule:
+    and:
+    - match:
+        parameter:
+          source: header
+          name: X-Hub-Signature
+        secret: mysecret
+        type: payload-hash-sha1

webhook_test.go

@@ -77,7 +77,7 @@ func TestWebhook(t *testing.T) {
 		for _, tt := range hookHandlerTests {
 			t.Run(tt.desc+"@"+hookTmpl, func(t *testing.T) {
 				ip, port := serverAddress(t)
-				args := []string{fmt.Sprintf("-hooks=%s", configPath), fmt.Sprintf("-ip=%s", ip), fmt.Sprintf("-port=%s", port), "-verbose"}
+				args := []string{fmt.Sprintf("-hooks=%s", configPath), fmt.Sprintf("-ip=%s", ip), fmt.Sprintf("-port=%s", port), "-debug"}
 
 				if len(tt.cliMethods) != 0 {
 					args = append(args, "-http-methods="+strings.Join(tt.cliMethods, ","))
@@ -111,6 +111,7 @@ func TestWebhook(t *testing.T) {
 				var res *http.Response
 
 				req.Header.Add("Content-Type", tt.contentType)
+				req.ContentLength = int64(len(tt.body))
 
 				client := &http.Client{}
 				res, err = client.Do(req)
@@ -663,6 +664,19 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00
 		``,
 	},
 
+	{
+		"empty-payload-signature", // allow empty payload signature validation
+		"empty-payload-signature",
+		nil,
+		"POST",
+		map[string]string{"X-Hub-Signature": "33f9d709782f62b8b4a0178586c65ab098a39fe2"},
+		"application/json",
+		``,
+		http.StatusOK,
+		``,
+		``,
+	},
+
 	// test with disallowed global HTTP method
 	{"global disallowed method", "bitbucket", []string{"Post "}, "GET", nil, `{}`, "application/json", http.StatusMethodNotAllowed, ``, ``},
 	// test with disallowed HTTP method