From 6d2f26d95283a77a97722adb57a2c6a2574576b3 Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Tue, 24 Nov 2020 21:11:45 -0600 Subject: [PATCH] Add soft signature failure support Add a new trigger-signature-soft-failures option to allow soft signature failures in Or rules. Fixes #234 --- docs/Hook-Definition.md | 1 + internal/hook/hook.go | 15 +- internal/hook/request.go | 3 + test/hooks.json.tmpl | 143 +++++++++++++++++ test/hooks.yaml.tmpl | 75 +++++++++ webhook.go | 3 + webhook_test.go | 320 ++++++++++++++++++++++++++++++++++++++- 7 files changed, 554 insertions(+), 6 deletions(-) diff --git a/docs/Hook-Definition.md b/docs/Hook-Definition.md index 925bea1..8a7e744 100644 --- a/docs/Hook-Definition.md +++ b/docs/Hook-Definition.md @@ -22,6 +22,7 @@ Hooks are defined as objects in the JSON or YAML hooks configuration file. Pleas * `pass-file-to-command` - specifies a list of entries that will be serialized as a file. Incoming [data](Referencing-Request-Values.md) will be serialized in a request-temporary-file (otherwise parallel calls of the hook would lead to concurrent overwritings of the file). The filename to be addressed within the subsequent script is provided via an environment variable. Use `envname` to specify the name of the environment variable. If `envname` is not provided `HOOK_` and the name used to reference the request value are used. Defining `command-working-directory` will store the file relative to this location, if not provided, the systems temporary file directory will be used. If `base64decode` is true, the incoming binary data will be base 64 decoded prior to storing it into the file. By default the corresponding file will be removed after the webhook exited. * `trigger-rule` - specifies the rule that will be evaluated in order to determine should the hook be triggered. Check [Hook rules page](Hook-Rules.md) to see the list of valid rules and their usage * `trigger-rule-mismatch-http-response-code` - specifies the HTTP status code to be returned when the trigger rule is not satisfied + * `trigger-signature-soft-failures` - allow signature validation failures within Or rules; by default, signature failures are treated as errors. ## Examples Check out [Hook examples page](Hook-Examples.md) for more complex examples of hooks. diff --git a/internal/hook/hook.go b/internal/hook/hook.go index 19809e7..c425179 100644 --- a/internal/hook/hook.go +++ b/internal/hook/hook.go @@ -95,6 +95,16 @@ func (e *SignatureError) Error() string { return fmt.Sprintf("invalid payload signature %s%s", e.Signature, empty) } +// IsSignatureError returns whether err is of type SignatureError. +func IsSignatureError(err error) bool { + switch err.(type) { + case *SignatureError: + return true + default: + return false + } +} + // ArgumentError describes an invalid argument passed to Hook. type ArgumentError struct { Argument Argument @@ -563,6 +573,7 @@ type Hook struct { JSONStringParameters []Argument `json:"parse-parameters-as-json,omitempty"` TriggerRule *Rules `json:"trigger-rule,omitempty"` TriggerRuleMismatchHttpResponseCode int `json:"trigger-rule-mismatch-http-response-code,omitempty"` + TriggerSignatureSoftFailures bool `json:"trigger-signature-soft-failures,omitempty"` IncomingPayloadContentType string `json:"incoming-payload-content-type,omitempty"` SuccessHttpResponseCode int `json:"success-http-response-code,omitempty"` HTTPMethods []string `json:"http-methods"` @@ -845,7 +856,9 @@ func (r OrRule) Evaluate(req *Request) (bool, error) { rv, err := v.Evaluate(req) if err != nil { if !IsParameterNodeError(err) { - return false, err + if !req.AllowSignatureErrors || (req.AllowSignatureErrors && !IsSignatureError(err)) { + return false, err + } } } diff --git a/internal/hook/request.go b/internal/hook/request.go index ef7c949..643d5a8 100644 --- a/internal/hook/request.go +++ b/internal/hook/request.go @@ -33,6 +33,9 @@ type Request struct { // The underlying HTTP request. RawRequest *http.Request + + // Treat signature errors as simple validate failures. + AllowSignatureErrors bool } func (r *Request) ParseJSONPayload() error { diff --git a/test/hooks.json.tmpl b/test/hooks.json.tmpl index fd5cb83..08d181a 100644 --- a/test/hooks.json.tmpl +++ b/test/hooks.json.tmpl @@ -55,6 +55,149 @@ ] } }, + { + "id": "github-multi-sig", + "execute-command": "{{ .Hookecho }}", + "command-working-directory": "/", + "http-methods": ["Post "], + "include-command-output-in-response": true, + "trigger-rule-mismatch-http-response-code": 400, + "trigger-signature-soft-failures": true, + "pass-environment-to-command": + [ + { + "source": "payload", + "name": "head_commit.timestamp" + } + ], + "pass-arguments-to-command": + [ + { + "source": "payload", + "name": "head_commit.id" + }, + { + "source": "payload", + "name": "head_commit.author.email" + } + ], + "trigger-rule": + { + "and": + [ + "or": + [ + { + "match": + { + "type": "payload-hmac-sha1", + "secret": "mysecretFAIL", + "parameter": + { + "source": "header", + "name": "X-Hub-Signature" + } + } + }, + { + "match": + { + "type": "payload-hmac-sha1", + "secret": "mysecret", + "parameter": + { + "source": "header", + "name": "X-Hub-Signature" + } + } + } + ], + { + "match": + { + "type": "value", + "value": "refs/heads/master", + "parameter": + { + "source": "payload", + "name": "ref" + } + } + } + ] + } + }, + { + "id": "github-multi-sig-fail", + "execute-command": "{{ .Hookecho }}", + "command-working-directory": "/", + "http-methods": ["Post "], + "include-command-output-in-response": true, + "trigger-rule-mismatch-http-response-code": 400, + "pass-environment-to-command": + [ + { + "source": "payload", + "name": "head_commit.timestamp" + } + ], + "pass-arguments-to-command": + [ + { + "source": "payload", + "name": "head_commit.id" + }, + { + "source": "payload", + "name": "head_commit.author.email" + } + ], + "trigger-rule": + { + "and": + [ + "or": + [ + { + "match": + { + "type": "payload-hmac-sha1", + "secret": "mysecretFAIL", + "parameter": + { + "source": "header", + "name": "X-Hub-Signature" + } + } + }, + { + "match": + { + "type": "payload-hmac-sha1", + "secret": "mysecret", + "parameter": + { + "source": "header", + "name": "X-Hub-Signature" + } + } + } + ], + { + "match": + { + "type": "value", + "value": "refs/heads/master", + "parameter": + { + "source": "payload", + "name": "ref" + } + } + } + ] + } + }, { "id": "bitbucket", "execute-command": "{{ .Hookecho }}", diff --git a/test/hooks.yaml.tmpl b/test/hooks.yaml.tmpl index 82833dc..163dc77 100644 --- a/test/hooks.yaml.tmpl +++ b/test/hooks.yaml.tmpl @@ -28,6 +28,81 @@ name: head_commit.timestamp command-working-directory: / +- id: github-multi-sig + http-methods: + - "Post " + trigger-rule: + and: + - or: + - match: + parameter: + source: header + name: X-Hub-Signature + secret: mysecretFAIL + type: payload-hmac-sha1 + - match: + parameter: + source: header + name: X-Hub-Signature + secret: mysecret + type: payload-hmac-sha1 + - match: + parameter: + source: payload + name: ref + type: value + value: refs/heads/master + include-command-output-in-response: true + trigger-rule-mismatch-http-response-code: 400 + trigger-signature-soft-failures: true + execute-command: '{{ .Hookecho }}' + pass-arguments-to-command: + - source: payload + name: head_commit.id + - source: payload + name: head_commit.author.email + pass-environment-to-command: + - source: payload + name: head_commit.timestamp + command-working-directory: / + +- id: github-multi-sig-fail + http-methods: + - "Post " + trigger-rule: + and: + - or: + - match: + parameter: + source: header + name: X-Hub-Signature + secret: mysecretFAIL + type: payload-hmac-sha1 + - match: + parameter: + source: header + name: X-Hub-Signature + secret: mysecret + type: payload-hmac-sha1 + - match: + parameter: + source: payload + name: ref + type: value + value: refs/heads/master + include-command-output-in-response: true + trigger-rule-mismatch-http-response-code: 400 + execute-command: '{{ .Hookecho }}' + pass-arguments-to-command: + - source: payload + name: head_commit.id + - source: payload + name: head_commit.author.email + pass-environment-to-command: + - source: payload + name: head_commit.timestamp + command-working-directory: / + - id: bitbucket trigger-rule: and: diff --git a/webhook.go b/webhook.go index ea6a9f2..ffbae4d 100644 --- a/webhook.go +++ b/webhook.go @@ -480,6 +480,9 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { if matchedHook.TriggerRule == nil { ok = true } else { + // Save signature soft failures option in request for evaluators + req.AllowSignatureErrors = matchedHook.TriggerSignatureSoftFailures + ok, err = matchedHook.TriggerRule.Evaluate(req) if err != nil { if !hook.IsParameterNodeError(err) { diff --git a/webhook_test.go b/webhook_test.go index 885efb6..7518300 100644 --- a/webhook_test.go +++ b/webhook_test.go @@ -133,10 +133,6 @@ func TestWebhook(t *testing.T) { { var bodyFailed bool - if res.StatusCode != tt.respStatus { - bodyFailed = true - } - if tt.bodyIsRE { bodyFailed = string(body) == tt.respBody } else { @@ -144,7 +140,7 @@ func TestWebhook(t *testing.T) { bodyFailed = !r.Match(body) } - if bodyFailed { + if res.StatusCode != tt.respStatus || bodyFailed { t.Errorf("failed %q (id: %s):\nexpected status: %#v, response: %s\ngot status: %#v, response: %s\ncommand output:\n%s\n", tt.desc, tt.id, tt.respStatus, tt.respBody, res.StatusCode, body, b) } } @@ -483,6 +479,320 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 `, ``, }, + { + "github-multi-sig", + "github-multi-sig", + nil, + "POST", + map[string]string{"X-Hub-Signature": "f68df0375d7b03e3eb29b4cf9f9ec12e08f42ff8"}, + "application/json", + `{ + "after":"1481a2de7b2a7d02428ad93446ab166be7793fbb", + "before":"17c497ccc7cca9c2f735aa07e9e3813060ce9a6a", + "commits":[ + { + "added":[ + + ], + "author":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "committer":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "distinct":true, + "id":"c441029cf673f84c8b7db52d0a5944ee5c52ff89", + "message":"Test", + "modified":[ + "README.md" + ], + "removed":[ + + ], + "timestamp":"2013-02-22T13:50:07-08:00", + "url":"https://github.com/octokitty/testing/commit/c441029cf673f84c8b7db52d0a5944ee5c52ff89" + }, + { + "added":[ + + ], + "author":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "committer":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "distinct":true, + "id":"36c5f2243ed24de58284a96f2a643bed8c028658", + "message":"This is me testing the windows client.", + "modified":[ + "README.md" + ], + "removed":[ + + ], + "timestamp":"2013-02-22T14:07:13-08:00", + "url":"https://github.com/octokitty/testing/commit/36c5f2243ed24de58284a96f2a643bed8c028658" + }, + { + "added":[ + "words/madame-bovary.txt" + ], + "author":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "committer":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "distinct":true, + "id":"1481a2de7b2a7d02428ad93446ab166be7793fbb", + "message":"Rename madame-bovary.txt to words/madame-bovary.txt", + "modified":[ + + ], + "removed":[ + "madame-bovary.txt" + ], + "timestamp":"2013-03-12T08:14:29-07:00", + "url":"https://github.com/octokitty/testing/commit/1481a2de7b2a7d02428ad93446ab166be7793fbb" + } + ], + "compare":"https://github.com/octokitty/testing/compare/17c497ccc7cc...1481a2de7b2a", + "created":false, + "deleted":false, + "forced":false, + "head_commit":{ + "added":[ + "words/madame-bovary.txt" + ], + "author":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "committer":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "distinct":true, + "id":"1481a2de7b2a7d02428ad93446ab166be7793fbb", + "message":"Rename madame-bovary.txt to words/madame-bovary.txt", + "modified":[ + + ], + "removed":[ + "madame-bovary.txt" + ], + "timestamp":"2013-03-12T08:14:29-07:00", + "url":"https://github.com/octokitty/testing/commit/1481a2de7b2a7d02428ad93446ab166be7793fbb" + }, + "pusher":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian" + }, + "ref":"refs/heads/master", + "repository":{ + "created_at":1332977768, + "description":"", + "fork":false, + "forks":0, + "has_downloads":true, + "has_issues":true, + "has_wiki":true, + "homepage":"", + "id":3860742, + "language":"Ruby", + "master_branch":"master", + "name":"testing", + "open_issues":2, + "owner":{ + "email":"lolwut@noway.biz", + "name":"octokitty" + }, + "private":false, + "pushed_at":1363295520, + "size":2156, + "stargazers":1, + "url":"https://github.com/octokitty/testing", + "watchers":1 + } + }`, + false, + http.StatusOK, + `arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz +env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 +`, + ``, + }, + { + "github-multi-sig-fail", + "github-multi-sig-fail", + nil, + "POST", + map[string]string{"X-Hub-Signature": "f68df0375d7b03e3eb29b4cf9f9ec12e08f42ff8"}, + "application/json", + `{ + "after":"1481a2de7b2a7d02428ad93446ab166be7793fbb", + "before":"17c497ccc7cca9c2f735aa07e9e3813060ce9a6a", + "commits":[ + { + "added":[ + + ], + "author":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "committer":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "distinct":true, + "id":"c441029cf673f84c8b7db52d0a5944ee5c52ff89", + "message":"Test", + "modified":[ + "README.md" + ], + "removed":[ + + ], + "timestamp":"2013-02-22T13:50:07-08:00", + "url":"https://github.com/octokitty/testing/commit/c441029cf673f84c8b7db52d0a5944ee5c52ff89" + }, + { + "added":[ + + ], + "author":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "committer":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "distinct":true, + "id":"36c5f2243ed24de58284a96f2a643bed8c028658", + "message":"This is me testing the windows client.", + "modified":[ + "README.md" + ], + "removed":[ + + ], + "timestamp":"2013-02-22T14:07:13-08:00", + "url":"https://github.com/octokitty/testing/commit/36c5f2243ed24de58284a96f2a643bed8c028658" + }, + { + "added":[ + "words/madame-bovary.txt" + ], + "author":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "committer":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "distinct":true, + "id":"1481a2de7b2a7d02428ad93446ab166be7793fbb", + "message":"Rename madame-bovary.txt to words/madame-bovary.txt", + "modified":[ + + ], + "removed":[ + "madame-bovary.txt" + ], + "timestamp":"2013-03-12T08:14:29-07:00", + "url":"https://github.com/octokitty/testing/commit/1481a2de7b2a7d02428ad93446ab166be7793fbb" + } + ], + "compare":"https://github.com/octokitty/testing/compare/17c497ccc7cc...1481a2de7b2a", + "created":false, + "deleted":false, + "forced":false, + "head_commit":{ + "added":[ + "words/madame-bovary.txt" + ], + "author":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "committer":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "distinct":true, + "id":"1481a2de7b2a7d02428ad93446ab166be7793fbb", + "message":"Rename madame-bovary.txt to words/madame-bovary.txt", + "modified":[ + + ], + "removed":[ + "madame-bovary.txt" + ], + "timestamp":"2013-03-12T08:14:29-07:00", + "url":"https://github.com/octokitty/testing/commit/1481a2de7b2a7d02428ad93446ab166be7793fbb" + }, + "pusher":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian" + }, + "ref":"refs/heads/master", + "repository":{ + "created_at":1332977768, + "description":"", + "fork":false, + "forks":0, + "has_downloads":true, + "has_issues":true, + "has_wiki":true, + "homepage":"", + "id":3860742, + "language":"Ruby", + "master_branch":"master", + "name":"testing", + "open_issues":2, + "owner":{ + "email":"lolwut@noway.biz", + "name":"octokitty" + }, + "private":false, + "pushed_at":1363295520, + "size":2156, + "stargazers":1, + "url":"https://github.com/octokitty/testing", + "watchers":1 + } + }`, + false, + http.StatusInternalServerError, + `Error occurred while evaluating hook rules.`, + ``, + }, { "bitbucket", // bitbucket sends their payload using uriencoded params. "bitbucket",