From 41ac427a895a80b744e1ab698350277121fd0e1f Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Thu, 21 May 2020 18:02:52 -0500 Subject: [PATCH] Warn on failed validate of empty payload signature If signature validation fails on an empty payload, append a note to the end of the error message. Updates #423 --- internal/hook/hook.go | 36 +++++++++++++++++++++++------------- internal/hook/hook_test.go | 27 ++++++++++++++++++++------- test/hooks.json.tmpl | 24 ++++++++++++++++++++++++ test/hooks.yaml.tmpl | 13 +++++++++++++ webhook_test.go | 16 +++++++++++++++- 5 files changed, 95 insertions(+), 21 deletions(-) diff --git a/internal/hook/hook.go b/internal/hook/hook.go index a9b4774..dd5546c 100644 --- a/internal/hook/hook.go +++ b/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 "" } - if e.Signatures != nil { - return fmt.Sprintf("invalid payload signatures %s", e.Signatures) + var empty string + 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)) { - return expectedMAC, err + if hmac.Equal([]byte(signature), []byte(actualMAC)) { + return actualMAC, err } } - return expectedMAC, &SignatureError{ - Signatures: signatures, + e := &SignatureError{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 } diff --git a/internal/hook/hook_test.go b/internal/hook/hook_test.go index 489fde2..e348930 100644 --- a/internal/hook/hook_test.go +++ b/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, }, } diff --git a/test/hooks.json.tmpl b/test/hooks.json.tmpl index 0c7fc0c..71673ce 100644 --- a/test/hooks.json.tmpl +++ b/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" + } + } + } + ] + } } ] diff --git a/test/hooks.yaml.tmpl b/test/hooks.yaml.tmpl index f1e1204..7c6e201 100644 --- a/test/hooks.yaml.tmpl +++ b/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 diff --git a/webhook_test.go b/webhook_test.go index ebf350d..6dda780 100644 --- a/webhook_test.go +++ b/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