From 85889fe37827fe015d27e4629775b0c4e5fda444 Mon Sep 17 00:00:00 2001 From: Adnan Hajdarevic Date: Thu, 21 Dec 2017 13:14:07 +0100 Subject: [PATCH 01/15] Fix nilpointer dereference when file cannot be created --- webhook.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/webhook.go b/webhook.go index fd3ce1d..79c2c7a 100644 --- a/webhook.go +++ b/webhook.go @@ -23,7 +23,7 @@ import ( ) const ( - version = "2.6.7" + version = "2.6.8" ) var ( @@ -368,13 +368,16 @@ func handleHook(h *hook.Hook, rid string, headers, query, payload *map[string]in tmpfile, err := ioutil.TempFile(h.CommandWorkingDirectory, files[i].EnvName) if err != nil { log.Printf("[%s] error creating temp file [%s]", rid, err) + continue } log.Printf("[%s] writing env %s file %s", rid, files[i].EnvName, tmpfile.Name()) if _, err := tmpfile.Write(files[i].Data); err != nil { log.Printf("[%s] error writing file %s [%s]", rid, tmpfile.Name(), err) + continue } if err := tmpfile.Close(); err != nil { log.Printf("[%s] error closing file %s [%s]", rid, tmpfile.Name(), err) + continue } files[i].File = tmpfile From a811db410bf321f0820f4464f51f371ebb012db1 Mon Sep 17 00:00:00 2001 From: Adnan Hajdarevic Date: Thu, 21 Dec 2017 13:25:19 +0100 Subject: [PATCH 02/15] check before removing --- webhook.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/webhook.go b/webhook.go index 79c2c7a..71fdc6b 100644 --- a/webhook.go +++ b/webhook.go @@ -397,10 +397,12 @@ func handleHook(h *hook.Hook, rid string, headers, query, payload *map[string]in } for i := range files { - log.Printf("[%s] removing file %s\n", rid, files[i].File.Name()) - err := os.Remove(files[i].File.Name()) - if err != nil { - log.Printf("[%s] error removing file %s [%s]", rid, files[i].File.Name(), err) + if files[i].File != nil { + log.Printf("[%s] removing file %s\n", rid, files[i].File.Name()) + err := os.Remove(files[i].File.Name()) + if err != nil { + log.Printf("[%s] error removing file %s [%s]", rid, files[i].File.Name(), err) + } } } From dcda096b5d8e5a359129199a33599496339e725f Mon Sep 17 00:00:00 2001 From: Hass_SEA Date: Thu, 18 Jan 2018 12:36:16 -0800 Subject: [PATCH 03/15] Update Rules Document with scalr-signature Updated Rules Document with scalr-signature information --- docs/Hook-Rules.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/docs/Hook-Rules.md b/docs/Hook-Rules.md index 32d35d3..274a0b5 100644 --- a/docs/Hook-Rules.md +++ b/docs/Hook-Rules.md @@ -198,4 +198,20 @@ The IP can be IPv4- or IPv6-formatted, using [CIDR notation](https://en.wikipedi "ip-range": "192.168.0.1/24" } } +``` + +### 5. Match scalr-signature + +The trigger rule checks the scalr signature and checks that the request was signed less than 5 minutes before it was received. +A unqiue signing key is generated for each webhook end point URL you register in Scalr +Given the time check make sure that NTP is enabled on both your Scalr and webhook server to prevent any issues + +```json +{ + "match": + { + "type": "scalr-signature", + "secret": "Scalr-provided signing key" + } +} ``` \ No newline at end of file From 3f8dbf09dcc980cc5adf8dad7ad02208dab42d41 Mon Sep 17 00:00:00 2001 From: Hass_SEA Date: Thu, 18 Jan 2018 12:40:05 -0800 Subject: [PATCH 04/15] Correct typos - Rules Document with scalr-signature Correct typos - Rules Document with scalr-signature --- docs/Hook-Rules.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/Hook-Rules.md b/docs/Hook-Rules.md index 274a0b5..52947aa 100644 --- a/docs/Hook-Rules.md +++ b/docs/Hook-Rules.md @@ -202,8 +202,8 @@ The IP can be IPv4- or IPv6-formatted, using [CIDR notation](https://en.wikipedi ### 5. Match scalr-signature -The trigger rule checks the scalr signature and checks that the request was signed less than 5 minutes before it was received. -A unqiue signing key is generated for each webhook end point URL you register in Scalr +The trigger rule checks the scalr signature and also checks that the request was signed less than 5 minutes before it was received. +A unqiue signing key is generated for each webhook endpoint URL you register in Scalr. Given the time check make sure that NTP is enabled on both your Scalr and webhook server to prevent any issues ```json From 6dc331726d24add54b30ef421d0415c8dcbdaa54 Mon Sep 17 00:00:00 2001 From: Hass_SEA Date: Thu, 18 Jan 2018 12:43:12 -0800 Subject: [PATCH 05/15] Updated Examples document with scalr-signature Updated the Examples document with an example of how you would use the scalr-signature match rule --- docs/Hook-Examples.md | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/docs/Hook-Examples.md b/docs/Hook-Examples.md index 308c8e3..aedba52 100644 --- a/docs/Hook-Examples.md +++ b/docs/Hook-Examples.md @@ -281,3 +281,44 @@ or in a single line, using https://github.com/jpmens/jo to generate the JSON cod jo binary=%filename.zip | curl -H "Content-Type:application/json" -X POST -d @- \ http://localhost:9000/hooks/test-file-webhook + + +## Incoming Scalr Webhook +[Guide by @hassanbabaie] +Scalr makes webhook calls based on an event to a configured webhook endpoint (for example Host Down, Host Up). Webhook endpoints are URLs where Scalr will deliver Webhook notifications. +Scalr assigns a unique signing key for every configured webhook endpoint. +Refer to this URL for information on how to setup the webhook call on the Scalr side: [Scalr Wiki Webhooks](https://scalr-wiki.atlassian.net/wiki/spaces/docs/pages/6193173/Webhooks) +In order to leverage the Signing Key for addtional authentication/security you must configure the trigger rule with a match type of "scalr-signature". + +```json +[ + { + "id": "redeploy-webhook", + "execute-command": "/home/adnan/redeploy-go-webhook.sh", + "command-working-directory": "/home/adnan/go", + "include-command-output-in-response": true, + "trigger-rule": + { + "match": + { + "type": "scalr-signature", + "secret": "Scalr-provided signing key" + } + }, + "pass-environment-to-command": + [ + { + "envname": "EVENT_NAME", + "source": "payload", + "name": "eventName" + }, + { + "envname": "SERVER_HOSTNAME", + "source": "payload", + "name": "data.SCALR_SERVER_HOSTNAME" + } + ] + } +] + +``` \ No newline at end of file From 0934b9414cda09e49cdc21fd2d81e000ee7d9d37 Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Wed, 14 Feb 2018 16:33:47 -0600 Subject: [PATCH 06/15] Add Travis CI and Appveyor configurations Fixes #226 --- .travis.yml | 21 +++++++++++++++++++++ appveyor.yml | 15 +++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 .travis.yml create mode 100644 appveyor.yml diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..ef89aae --- /dev/null +++ b/.travis.yml @@ -0,0 +1,21 @@ +language: go + +go: + - 1.8.x + - 1.9.x + - tip + +os: + - linux + - osx + +matrix: + fast_finish: true + allow_failures: + - go: tip + +install: + - go get -d -v -t ./... + +script: + - go test -v -race ./... diff --git a/appveyor.yml b/appveyor.yml new file mode 100644 index 0000000..ef2d980 --- /dev/null +++ b/appveyor.yml @@ -0,0 +1,15 @@ +clone_folder: C:\gopath\src\github.com\adnanh\webhook + +environment: + GOPATH: c:\gopath + +install: + - set PATH=%PATH%;C:\MinGW\bin;%GOPATH%\bin + - echo %PATH% + - echo %GOPATH% + - go version + - go env + - go get -d -v -t ./... + +build_script: + - go test -v -race ./... From 0feeb945fc4d8a17685d106b02a7315805c6761b Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Thu, 15 Feb 2018 19:15:08 -0600 Subject: [PATCH 07/15] Fix some tests for Windows This commit incorporates some tests into the main TestWebhook framework. New features to TestWebhook: - Check log output against Regexp - Add Testing sub-tests Updates #225 --- test/hooks.json.tmpl | 24 +++++ test/hooks.yaml.tmpl | 29 ++++-- webhook_test.go | 216 ++++++++++++++++++++----------------------- 3 files changed, 149 insertions(+), 120 deletions(-) diff --git a/test/hooks.json.tmpl b/test/hooks.json.tmpl index d65d1d3..2fd90da 100644 --- a/test/hooks.json.tmpl +++ b/test/hooks.json.tmpl @@ -180,5 +180,29 @@ "execute-command": "{{ .Hookecho }}", "include-command-output-in-response": true, "include-command-output-in-response-on-error": true + }, + { + "id": "static-params-ok", + "execute-command": "{{ .Hookecho }}", + "response-message": "success", + "include-command-output-in-response": true, + "pass-arguments-to-command": [ + { + "source": "string", + "name": "passed" + } + ], + }, + { + "id": "warn-on-space", + "execute-command": "{{ .Hookecho }} foo", + "response-message": "success", + "include-command-output-in-response": true, + "pass-arguments-to-command": [ + { + "source": "string", + "name": "passed" + } + ], } ] diff --git a/test/hooks.yaml.tmpl b/test/hooks.yaml.tmpl index c0377e5..c2fffcd 100644 --- a/test/hooks.yaml.tmpl +++ b/test/hooks.yaml.tmpl @@ -1,4 +1,5 @@ -- trigger-rule: +- id: github + trigger-rule: and: - match: parameter: @@ -23,9 +24,10 @@ pass-environment-to-command: - source: payload name: head_commit.timestamp - id: github command-working-directory: / -- trigger-rule: + +- id: bitbucket + trigger-rule: and: - match: parameter: @@ -52,9 +54,10 @@ execute-command: '{{ .Hookecho }}' response-message: success include-command-output-in-response: false - id: bitbucket command-working-directory: / -- trigger-rule: + +- id: gitlab + trigger-rule: match: parameter: source: payload @@ -71,25 +74,28 @@ execute-command: '{{ .Hookecho }}' response-message: success include-command-output-in-response: true - id: gitlab command-working-directory: / + - id: capture-command-output-on-success-not-by-default pass-arguments-to-command: - source: string name: exit=0 execute-command: '{{ .Hookecho }}' + - id: capture-command-output-on-success-yes-with-flag pass-arguments-to-command: - source: string name: exit=0 execute-command: '{{ .Hookecho }}' include-command-output-in-response: true + - id: capture-command-output-on-error-not-by-default pass-arguments-to-command: - source: string name: exit=1 execute-command: '{{ .Hookecho }}' include-command-output-in-response: true + - id: capture-command-output-on-error-yes-with-extra-flag pass-arguments-to-command: - source: string @@ -97,3 +103,14 @@ execute-command: '{{ .Hookecho }}' include-command-output-in-response: true include-command-output-in-response-on-error: true + +- id: static-params-ok + execute-command: '{{ .Hookecho }}' + include-command-output-in-response: true + pass-arguments-to-command: + - source: string + name: passed + +- id: warn-on-space + execute-command: '{{ .Hookecho }} foo' + include-command-output-in-response: true \ No newline at end of file diff --git a/webhook_test.go b/webhook_test.go index 4373b16..4e04ab3 100644 --- a/webhook_test.go +++ b/webhook_test.go @@ -21,15 +21,26 @@ import ( ) func TestStaticParams(t *testing.T) { + // FIXME(moorereason): incorporate this test into TestWebhook. + // Need to be able to execute a binary with a space in the filename. + if runtime.GOOS == "windows" { + t.Skip("Skipping on Windows") + } spHeaders := make(map[string]interface{}) spHeaders["User-Agent"] = "curl/7.54.0" spHeaders["Accept"] = "*/*" - // case 1: correct entry + // case 2: binary with spaces in its name + err := os.Symlink("/bin/echo", "/tmp/with space") + if err != nil { + t.Fatalf("%v", err) + } + defer os.Remove("/tmp/with space") + spHook := &hook.Hook{ - ID: "static-params-ok", - ExecuteCommand: "/bin/echo", + ID: "static-params-name-space", + ExecuteCommand: "/tmp/with space", CommandWorkingDirectory: "/tmp", ResponseMessage: "success", CaptureCommandOutput: true, @@ -41,68 +52,14 @@ func TestStaticParams(t *testing.T) { b := &bytes.Buffer{} log.SetOutput(b) - s, err := handleHook(spHook, "test", &spHeaders, &map[string]interface{}{}, &map[string]interface{}{}, &[]byte{}) + _, err = handleHook(spHook, "test", &spHeaders, &map[string]interface{}{}, &map[string]interface{}{}, &[]byte{}) if err != nil { t.Fatalf("Unexpected error: %v\n", err) } - matched, _ := regexp.MatchString("(?s).*command output: passed.*static-params-ok", b.String()) - if !matched { - t.Fatalf("Unexpected log output:\n%s", b) - } - - // case 2: binary with spaces in its name - err = os.Symlink("/bin/echo", "/tmp/with space") - if err != nil { - t.Fatalf("%v", err) - } - defer os.Remove("/tmp/with space") - - spHook = &hook.Hook{ - ID: "static-params-name-space", - ExecuteCommand: "/tmp/with space", - CommandWorkingDirectory: "/tmp", - ResponseMessage: "success", - CaptureCommandOutput: true, - PassArgumentsToCommand: []hook.Argument{ - hook.Argument{Source: "string", Name: "passed"}, - }, - } - - b = &bytes.Buffer{} - log.SetOutput(b) - - s, err = handleHook(spHook, "test", &spHeaders, &map[string]interface{}{}, &map[string]interface{}{}, &[]byte{}) - if err != nil { - t.Fatalf("Unexpected error: %v\n", err) - } - matched, _ = regexp.MatchString("(?s)command output: .*static-params-name-space", b.String()) + matched, _ := regexp.MatchString("(?s)command output: .*static-params-name-space", b.String()) if !matched { t.Fatalf("Unexpected log output:\n%sn", b) } - - // case 3: parameters specified in execute-command - spHook = &hook.Hook{ - ID: "static-params-bad", - ExecuteCommand: "/bin/echo success", - CommandWorkingDirectory: "/tmp", - ResponseMessage: "success", - CaptureCommandOutput: true, - PassArgumentsToCommand: []hook.Argument{ - hook.Argument{Source: "string", Name: "failed"}, - }, - } - - b = &bytes.Buffer{} - log.SetOutput(b) - - s, err = handleHook(spHook, "test", &spHeaders, &map[string]interface{}{}, &map[string]interface{}{}, &[]byte{}) - if err == nil { - t.Fatalf("Error expected, but none returned: %s\n", s) - } - matched, _ = regexp.MatchString("(?s)unable to locate command: ..bin.echo success.*use 'pass-arguments-to-command'", b.String()) - if !matched { - t.Fatalf("Unexpected log output:\n%s\n", b) - } } func TestWebhook(t *testing.T) { @@ -110,17 +67,20 @@ func TestWebhook(t *testing.T) { defer cleanupHookecho() for _, hookTmpl := range []string{"test/hooks.json.tmpl", "test/hooks.yaml.tmpl"} { - config, cleanupConfig := genConfig(t, hookecho, hookTmpl) - defer cleanupConfig() + configPath, cleanupConfigFn := genConfig(t, hookecho, hookTmpl) + defer cleanupConfigFn() - webhook, cleanupWebhook := buildWebhook(t) - defer cleanupWebhook() + webhook, cleanupWebhookFn := buildWebhook(t) + defer cleanupWebhookFn() ip, port := serverAddress(t) - args := []string{fmt.Sprintf("-hooks=%s", config), 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), "-verbose"} + + // Setup a buffer for capturing webhook logs for later evaluation + b := &bytes.Buffer{} cmd := exec.Command(webhook, args...) - //cmd.Stderr = os.Stderr // uncomment to see verbose output + cmd.Stderr = b cmd.Env = webhookEnv() cmd.Args[0] = "webhook" if err := cmd.Start(); err != nil { @@ -131,69 +91,83 @@ func TestWebhook(t *testing.T) { waitForServerReady(t, ip, port) for _, tt := range hookHandlerTests { - url := fmt.Sprintf("http://%s:%s/hooks/%s", ip, port, tt.id) + t.Run(tt.desc, func(t *testing.T) { - req, err := http.NewRequest("POST", url, ioutil.NopCloser(strings.NewReader(tt.body))) - if err != nil { - t.Errorf("New request failed: %s", err) - } + url := fmt.Sprintf("http://%s:%s/hooks/%s", ip, port, tt.id) - for k, v := range tt.headers { - req.Header.Add(k, v) - } + req, err := http.NewRequest("POST", url, ioutil.NopCloser(strings.NewReader(tt.body))) + if err != nil { + t.Errorf("New request failed: %s", err) + } - var res *http.Response + for k, v := range tt.headers { + req.Header.Add(k, v) + } - if tt.urlencoded == true { - req.Header.Add("Content-Type", "application/x-www-form-urlencoded") - } else { - req.Header.Add("Content-Type", "application/json") - } + var res *http.Response - client := &http.Client{} - res, err = client.Do(req) - if err != nil { - t.Errorf("client.Do failed: %s", err) - } + if tt.urlencoded == true { + req.Header.Add("Content-Type", "application/x-www-form-urlencoded") + } else { + req.Header.Add("Content-Type", "application/json") + } - body, err := ioutil.ReadAll(res.Body) - res.Body.Close() - if err != nil { - t.Errorf("POST %q: failed to ready body: %s", tt.desc, err) - } + client := &http.Client{} + res, err = client.Do(req) + if err != nil { + t.Errorf("client.Do failed: %s", err) + } - if res.StatusCode != tt.respStatus || string(body) != tt.respBody { - t.Errorf("failed %q (id: %s):\nexpected status: %#v, response: %s\ngot status: %#v, response: %s", tt.desc, tt.id, tt.respStatus, tt.respBody, res.StatusCode, body) - } + body, err := ioutil.ReadAll(res.Body) + res.Body.Close() + if err != nil { + t.Errorf("POST %q: failed to ready body: %s", tt.desc, err) + } + + if res.StatusCode != tt.respStatus || string(body) != tt.respBody { + t.Errorf("failed %q (id: %s):\nexpected status: %#v, response: %s\ngot status: %#v, response: %s", tt.desc, tt.id, tt.respStatus, tt.respBody, res.StatusCode, body) + } + + if tt.logMatch == "" { + b.Reset() + return + } + + matched, _ := regexp.MatchString(tt.logMatch, b.String()) + if !matched { + t.Fatalf("Unexpected log output:\n%s", b) + } + b.Reset() + }) } } } -func buildHookecho(t *testing.T) (bin string, cleanup func()) { +func buildHookecho(t *testing.T) (binPath string, cleanupFn func()) { tmp, err := ioutil.TempDir("", "hookecho-test-") if err != nil { t.Fatal(err) } defer func() { - if cleanup == nil { + if cleanupFn == nil { os.RemoveAll(tmp) } }() - bin = filepath.Join(tmp, "hookecho") + binPath = filepath.Join(tmp, "hookecho") if runtime.GOOS == "windows" { - bin += ".exe" + binPath += ".exe" } - cmd := exec.Command("go", "build", "-o", bin, "test/hookecho.go") + cmd := exec.Command("go", "build", "-o", binPath, "test/hookecho.go") if err := cmd.Run(); err != nil { t.Fatalf("Building hookecho: %v", err) } - return bin, func() { os.RemoveAll(tmp) } + return binPath, func() { os.RemoveAll(tmp) } } -func genConfig(t *testing.T, bin string, hookTemplate string) (config string, cleanup func()) { +func genConfig(t *testing.T, bin string, hookTemplate string) (configPath string, cleanupFn func()) { tmpl := template.Must(template.ParseFiles(hookTemplate)) tmp, err := ioutil.TempDir("", "webhook-config-") @@ -201,7 +175,7 @@ func genConfig(t *testing.T, bin string, hookTemplate string) (config string, cl t.Fatal(err) } defer func() { - if cleanup == nil { + if cleanupFn == nil { os.RemoveAll(tmp) } }() @@ -215,7 +189,11 @@ func genConfig(t *testing.T, bin string, hookTemplate string) (config string, cl } defer file.Close() - data := struct{ Hookecho string }{filepath.ToSlash(bin)} + data := struct{ Hookecho string }{filepath.FromSlash(bin)} + if runtime.GOOS == "windows" { + // Simulate escaped backslashes on Windows. + data.Hookecho = strings.Replace(data.Hookecho, `\`, `\\`, -1) + } if err := tmpl.Execute(file, data); err != nil { t.Fatalf("Executing template: %v", err) } @@ -223,28 +201,28 @@ func genConfig(t *testing.T, bin string, hookTemplate string) (config string, cl return path, func() { os.RemoveAll(tmp) } } -func buildWebhook(t *testing.T) (bin string, cleanup func()) { +func buildWebhook(t *testing.T) (binPath string, cleanupFn func()) { tmp, err := ioutil.TempDir("", "webhook-test-") if err != nil { t.Fatal(err) } defer func() { - if cleanup == nil { + if cleanupFn == nil { os.RemoveAll(tmp) } }() - bin = filepath.Join(tmp, "webhook") + binPath = filepath.Join(tmp, "webhook") if runtime.GOOS == "windows" { - bin += ".exe" + binPath += ".exe" } - cmd := exec.Command("go", "build", "-o", bin) + cmd := exec.Command("go", "build", "-o", binPath) if err := cmd.Run(); err != nil { t.Fatalf("Building webhook: %v", err) } - return bin, func() { os.RemoveAll(tmp) } + return binPath, func() { os.RemoveAll(tmp) } } func serverAddress(t *testing.T) (string, string) { @@ -313,6 +291,7 @@ var hookHandlerTests = []struct { respStatus int respBody string + logMatch string }{ { "github", @@ -467,6 +446,7 @@ var hookHandlerTests = []struct { `arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 `, + ``, }, { "bitbucket", // bitbucket sends their payload using uriencoded params. @@ -476,6 +456,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 true, http.StatusOK, `success`, + ``, }, { "gitlab", @@ -527,6 +508,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 http.StatusOK, `arg: b6568db1bc1dcd7f8b4d5a946b0b91f9dacd7327 John Smith john@example.com `, + ``, }, { @@ -567,6 +549,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 `arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 `, + ``, }, { @@ -605,20 +588,25 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 http.StatusOK, `arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz `, + ``, }, // test with custom return code - {"empty payload", "github", nil, `{}`, false, http.StatusBadRequest, `Hook rules were not satisfied.`}, + {"empty payload", "github", nil, `{}`, false, http.StatusBadRequest, `Hook rules were not satisfied.`, ``}, // test with custom invalid http code, should default to 200 OK - {"empty payload", "bitbucket", nil, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`}, + {"empty payload", "bitbucket", nil, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`, ``}, // test with no configured http return code, should default to 200 OK - {"empty payload", "gitlab", nil, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`}, + {"empty payload", "gitlab", nil, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`, ``}, // test capturing command output - {"don't capture output on success by default", "capture-command-output-on-success-not-by-default", nil, `{}`, false, http.StatusOK, ``}, + {"don't capture output on success by default", "capture-command-output-on-success-not-by-default", nil, `{}`, false, http.StatusOK, ``, ``}, {"capture output on success with flag set", "capture-command-output-on-success-yes-with-flag", nil, `{}`, false, http.StatusOK, `arg: exit=0 -`}, - {"don't capture output on error by default", "capture-command-output-on-error-not-by-default", nil, `{}`, false, http.StatusInternalServerError, `Error occurred while executing the hook's command. Please check your logs for more details.`}, +`, ``}, + {"don't capture output on error by default", "capture-command-output-on-error-not-by-default", nil, `{}`, false, http.StatusInternalServerError, `Error occurred while executing the hook's command. Please check your logs for more details.`, ``}, {"capture output on error with extra flag set", "capture-command-output-on-error-yes-with-extra-flag", nil, `{}`, false, http.StatusInternalServerError, `arg: exit=1 -`}, +`, ``}, + + // Check logs + {"static params should pass", "static-params-ok", nil, `{}`, false, http.StatusOK, "arg: passed\n", `(?s)command output: arg: passed`}, + {"command with space logs warning", "warn-on-space", nil, `{}`, false, http.StatusInternalServerError, "Error occurred while executing the hook's command. Please check your logs for more details.", `(?s)unable to locate command.*use 'pass[-]arguments[-]to[-]command' to specify args`}, } From 337621998e791cdd6cc90ca96780e1a102c42ed8 Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Thu, 15 Feb 2018 19:53:28 -0600 Subject: [PATCH 08/15] Fix race in TestWebhook Previous commit misused a bytes.Buffer. Protect the buffer with a mutex. --- webhook_test.go | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/webhook_test.go b/webhook_test.go index 4e04ab3..0c3016a 100644 --- a/webhook_test.go +++ b/webhook_test.go @@ -13,6 +13,7 @@ import ( "regexp" "runtime" "strings" + "sync" "testing" "text/template" "time" @@ -66,18 +67,18 @@ func TestWebhook(t *testing.T) { hookecho, cleanupHookecho := buildHookecho(t) defer cleanupHookecho() + webhook, cleanupWebhookFn := buildWebhook(t) + defer cleanupWebhookFn() + for _, hookTmpl := range []string{"test/hooks.json.tmpl", "test/hooks.yaml.tmpl"} { configPath, cleanupConfigFn := genConfig(t, hookecho, hookTmpl) defer cleanupConfigFn() - webhook, cleanupWebhookFn := buildWebhook(t) - defer cleanupWebhookFn() - ip, port := serverAddress(t) args := []string{fmt.Sprintf("-hooks=%s", configPath), fmt.Sprintf("-ip=%s", ip), fmt.Sprintf("-port=%s", port), "-verbose"} // Setup a buffer for capturing webhook logs for later evaluation - b := &bytes.Buffer{} + b := &buffer{} cmd := exec.Command(webhook, args...) cmd.Stderr = b @@ -610,3 +611,33 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 {"static params should pass", "static-params-ok", nil, `{}`, false, http.StatusOK, "arg: passed\n", `(?s)command output: arg: passed`}, {"command with space logs warning", "warn-on-space", nil, `{}`, false, http.StatusInternalServerError, "Error occurred while executing the hook's command. Please check your logs for more details.", `(?s)unable to locate command.*use 'pass[-]arguments[-]to[-]command' to specify args`}, } + +// buffer provides a concurrency-safe bytes.Buffer to tests above. +type buffer struct { + b bytes.Buffer + m sync.Mutex +} + +func (b *buffer) Read(p []byte) (n int, err error) { + b.m.Lock() + defer b.m.Unlock() + return b.b.Read(p) +} + +func (b *buffer) Write(p []byte) (n int, err error) { + b.m.Lock() + defer b.m.Unlock() + return b.b.Write(p) +} + +func (b *buffer) String() string { + b.m.Lock() + defer b.m.Unlock() + return b.b.String() +} + +func (b *buffer) Reset() { + b.m.Lock() + defer b.m.Unlock() + b.b.Reset() +} From 471c849c503f35527fa4b7cfb6a64c979a1888ee Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Fri, 16 Feb 2018 14:36:08 -0600 Subject: [PATCH 09/15] Fix another race condition in TestWebhook There's the potential for a race condition where we try to read the logs buffer before the logs have been flushed by the webhook process. Kill the process to flush the logs before testing against the log buffer. --- webhook_test.go | 48 ++++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/webhook_test.go b/webhook_test.go index 0c3016a..6113b9b 100644 --- a/webhook_test.go +++ b/webhook_test.go @@ -74,26 +74,26 @@ func TestWebhook(t *testing.T) { configPath, cleanupConfigFn := genConfig(t, hookecho, hookTmpl) defer cleanupConfigFn() - ip, port := serverAddress(t) - args := []string{fmt.Sprintf("-hooks=%s", configPath), fmt.Sprintf("-ip=%s", ip), fmt.Sprintf("-port=%s", port), "-verbose"} - - // Setup a buffer for capturing webhook logs for later evaluation - b := &buffer{} - - cmd := exec.Command(webhook, args...) - cmd.Stderr = b - cmd.Env = webhookEnv() - cmd.Args[0] = "webhook" - if err := cmd.Start(); err != nil { - t.Fatalf("failed to start webhook: %s", err) - } - defer killAndWait(cmd) - - waitForServerReady(t, ip, port) - for _, tt := range hookHandlerTests { t.Run(tt.desc, 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"} + + // Setup a buffer for capturing webhook logs for later evaluation + b := &buffer{} + + cmd := exec.Command(webhook, args...) + cmd.Stderr = b + cmd.Env = webhookEnv() + cmd.Args[0] = "webhook" + if err := cmd.Start(); err != nil { + t.Fatalf("failed to start webhook: %s", err) + } + defer killAndWait(cmd) + + waitForServerReady(t, ip, port) + url := fmt.Sprintf("http://%s:%s/hooks/%s", ip, port, tt.id) req, err := http.NewRequest("POST", url, ioutil.NopCloser(strings.NewReader(tt.body))) @@ -130,15 +130,19 @@ func TestWebhook(t *testing.T) { } if tt.logMatch == "" { - b.Reset() return } + // There's the potential for a race condition below where we + // try to read the logs buffer b before the logs have been + // flushed by the webhook process. Kill the process to flush + // the logs. + killAndWait(cmd) + matched, _ := regexp.MatchString(tt.logMatch, b.String()) if !matched { - t.Fatalf("Unexpected log output:\n%s", b) + t.Errorf("failed log match for %q (id: %s):\nmatch pattern: %q\ngot:\n%s", tt.desc, tt.id, tt.logMatch, b) } - b.Reset() }) } } @@ -267,6 +271,10 @@ func waitForServer(t *testing.T, url string, status int, timeout time.Duration) } func killAndWait(cmd *exec.Cmd) { + if cmd == nil || cmd.ProcessState != nil && cmd.ProcessState.Exited() { + return + } + cmd.Process.Kill() cmd.Wait() } From cfed5cfe4be703bb9d8352b39fdef6e460336981 Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Fri, 16 Feb 2018 20:18:40 -0600 Subject: [PATCH 10/15] Fix unnecessary use of printf --- webhook.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webhook.go b/webhook.go index 71fdc6b..e8a9c00 100644 --- a/webhook.go +++ b/webhook.go @@ -264,7 +264,7 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { ok, err = matchedHook.TriggerRule.Evaluate(&headers, &query, &payload, &body, r.RemoteAddr) if err != nil { msg := fmt.Sprintf("[%s] error evaluating hook: %s", rid, err) - log.Printf(msg) + log.Print(msg) w.WriteHeader(http.StatusInternalServerError) fmt.Fprintf(w, "Error occurred while evaluating hook rules.") return From 48061f150832170a7baf4a686b87d44a74f350de Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Fri, 16 Feb 2018 20:23:25 -0600 Subject: [PATCH 11/15] Simplify boolean some comparisons --- hook/hook.go | 4 ++-- webhook.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/hook/hook.go b/hook/hook.go index fb1b6fc..434d7f7 100644 --- a/hook/hook.go +++ b/hook/hook.go @@ -687,7 +687,7 @@ func (r AndRule) Evaluate(headers, query, payload *map[string]interface{}, body } res = res && rv - if res == false { + if !res { return res, nil } } @@ -709,7 +709,7 @@ func (r OrRule) Evaluate(headers, query, payload *map[string]interface{}, body * } res = res || rv - if res == true { + if res { return res, nil } } diff --git a/webhook.go b/webhook.go index e8a9c00..3014b1c 100644 --- a/webhook.go +++ b/webhook.go @@ -120,7 +120,7 @@ func main() { newHooksFiles := hooksFiles[:0] for _, filePath := range hooksFiles { - if _, ok := loadedHooksFromFiles[filePath]; ok == true { + if _, ok := loadedHooksFromFiles[filePath]; ok { newHooksFiles = append(newHooksFiles, filePath) } } @@ -436,7 +436,7 @@ func reloadHooks(hooksFilePath string) { } } - if (matchLoadedHook(hook.ID) != nil && !wasHookIDAlreadyLoaded) || seenHooksIds[hook.ID] == true { + if (matchLoadedHook(hook.ID) != nil && !wasHookIDAlreadyLoaded) || seenHooksIds[hook.ID] { log.Printf("error: hook with the id %s has already been loaded!\nplease check your hooks file for duplicate hooks ids!", hook.ID) log.Println("reverting hooks back to the previous configuration") return From 8d260c6a7ebaa94b15f16adc85fa50f22aff839f Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Fri, 16 Feb 2018 20:26:33 -0600 Subject: [PATCH 12/15] Apply gofmt --- hook/hook.go | 84 +++++++++++++++++++++---------------------- hook/hook_test.go | 92 +++++++++++++++++++++++------------------------ test/hookecho.go | 2 +- 3 files changed, 89 insertions(+), 89 deletions(-) diff --git a/hook/hook.go b/hook/hook.go index 434d7f7..f5e20ad 100644 --- a/hook/hook.go +++ b/hook/hook.go @@ -11,8 +11,8 @@ import ( "errors" "fmt" "io/ioutil" - "math" "log" + "math" "net" "net/textproto" "os" @@ -129,44 +129,44 @@ func CheckPayloadSignature256(payload []byte, secret string, signature string) ( } return expectedMAC, err } - -func CheckScalrSignature(headers map[string]interface{}, body []byte, signingKey string, checkDate bool) (bool, error) { - // Check for the signature and date headers - if _, ok := headers["X-Signature"]; !ok { - return false, nil - } - if _, ok := headers["Date"]; !ok { - return false, nil - } - providedSignature := headers["X-Signature"].(string) - dateHeader := headers["Date"].(string) - mac := hmac.New(sha1.New, []byte(signingKey)) - mac.Write(body) - mac.Write([]byte(dateHeader)) - expectedSignature := hex.EncodeToString(mac.Sum(nil)) - - if !hmac.Equal([]byte(providedSignature), []byte(expectedSignature)) { - return false, &SignatureError{providedSignature} - } - - if !checkDate { - return true, nil - } - // Example format: Fri 08 Sep 2017 11:24:32 UTC - date, err := time.Parse("Mon 02 Jan 2006 15:04:05 MST", dateHeader) - //date, err := time.Parse(time.RFC1123, dateHeader) - if err != nil { - return false, err - } - now := time.Now() - delta := math.Abs(now.Sub(date).Seconds()) - - if delta > 300 { - return false, &SignatureError{"outdated"} - } - return true, nil - } - + +func CheckScalrSignature(headers map[string]interface{}, body []byte, signingKey string, checkDate bool) (bool, error) { + // Check for the signature and date headers + if _, ok := headers["X-Signature"]; !ok { + return false, nil + } + if _, ok := headers["Date"]; !ok { + return false, nil + } + providedSignature := headers["X-Signature"].(string) + dateHeader := headers["Date"].(string) + mac := hmac.New(sha1.New, []byte(signingKey)) + mac.Write(body) + mac.Write([]byte(dateHeader)) + expectedSignature := hex.EncodeToString(mac.Sum(nil)) + + if !hmac.Equal([]byte(providedSignature), []byte(expectedSignature)) { + return false, &SignatureError{providedSignature} + } + + if !checkDate { + return true, nil + } + // Example format: Fri 08 Sep 2017 11:24:32 UTC + date, err := time.Parse("Mon 02 Jan 2006 15:04:05 MST", dateHeader) + //date, err := time.Parse(time.RFC1123, dateHeader) + if err != nil { + return false, err + } + now := time.Now() + delta := math.Abs(now.Sub(date).Seconds()) + + if delta > 300 { + return false, &SignatureError{"outdated"} + } + return true, nil +} + // 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) { @@ -751,10 +751,10 @@ func (r MatchRule) Evaluate(headers, query, payload *map[string]interface{}, bod if r.Type == IPWhitelist { return CheckIPWhitelist(remoteAddr, r.IPRange) } - if r.Type == ScalrSignature { - return CheckScalrSignature(*headers, *body, r.Secret, true) + if r.Type == ScalrSignature { + return CheckScalrSignature(*headers, *body, r.Secret, true) } - + if arg, ok := r.Parameter.Get(headers, query, payload); ok { switch r.Type { case MatchValue: diff --git a/hook/hook_test.go b/hook/hook_test.go index d1134f1..1794a7c 100644 --- a/hook/hook_test.go +++ b/hook/hook_test.go @@ -60,52 +60,52 @@ func TestCheckPayloadSignature256(t *testing.T) { } } -var checkScalrSignatureTests = []struct { - description string - headers map[string]interface{} - payload []byte - secret string - expectedSignature string - ok bool -}{ - { - "Valid signature", - map[string]interface{}{"Date": "Thu 07 Sep 2017 06:30:04 UTC", "X-Signature": "48e395e38ac48988929167df531eb2da00063a7d"}, - []byte(`{"a": "b"}`), "bilFGi4ZVZUdG+C6r0NIM9tuRq6PaG33R3eBUVhLwMAErGBaazvXe4Gq2DcJs5q+", - "48e395e38ac48988929167df531eb2da00063a7d", true, - }, - { - "Wrong signature", - map[string]interface{}{"Date": "Thu 07 Sep 2017 06:30:04 UTC", "X-Signature": "999395e38ac48988929167df531eb2da00063a7d"}, - []byte(`{"a": "b"}`), "bilFGi4ZVZUdG+C6r0NIM9tuRq6PaG33R3eBUVhLwMAErGBaazvXe4Gq2DcJs5q+", - "48e395e38ac48988929167df531eb2da00063a7d", false, - }, - { - "Missing Date header", - map[string]interface{}{"X-Signature": "999395e38ac48988929167df531eb2da00063a7d"}, - []byte(`{"a": "b"}`), "bilFGi4ZVZUdG+C6r0NIM9tuRq6PaG33R3eBUVhLwMAErGBaazvXe4Gq2DcJs5q+", - "48e395e38ac48988929167df531eb2da00063a7d", false, - }, - { - "Missing X-Signature header", - map[string]interface{}{"Date": "Thu 07 Sep 2017 06:30:04 UTC"}, - []byte(`{"a": "b"}`), "bilFGi4ZVZUdG+C6r0NIM9tuRq6PaG33R3eBUVhLwMAErGBaazvXe4Gq2DcJs5q+", - "48e395e38ac48988929167df531eb2da00063a7d", false, - }, -} - -func TestCheckScalrSignature(t *testing.T) { - for _, testCase := range checkScalrSignatureTests { - valid, err := CheckScalrSignature(testCase.headers, testCase.payload, testCase.secret, false) - if valid != testCase.ok { - t.Errorf("failed to check scalr signature fot test case: %s\nexpected ok:%#v, got ok:%#v}", - testCase.description, testCase.ok, valid) - } - - if err != nil && strings.Contains(err.Error(), testCase.expectedSignature) { - t.Errorf("error message should not disclose expected mac: %s on test case %s", err, testCase.description) - } - } +var checkScalrSignatureTests = []struct { + description string + headers map[string]interface{} + payload []byte + secret string + expectedSignature string + ok bool +}{ + { + "Valid signature", + map[string]interface{}{"Date": "Thu 07 Sep 2017 06:30:04 UTC", "X-Signature": "48e395e38ac48988929167df531eb2da00063a7d"}, + []byte(`{"a": "b"}`), "bilFGi4ZVZUdG+C6r0NIM9tuRq6PaG33R3eBUVhLwMAErGBaazvXe4Gq2DcJs5q+", + "48e395e38ac48988929167df531eb2da00063a7d", true, + }, + { + "Wrong signature", + map[string]interface{}{"Date": "Thu 07 Sep 2017 06:30:04 UTC", "X-Signature": "999395e38ac48988929167df531eb2da00063a7d"}, + []byte(`{"a": "b"}`), "bilFGi4ZVZUdG+C6r0NIM9tuRq6PaG33R3eBUVhLwMAErGBaazvXe4Gq2DcJs5q+", + "48e395e38ac48988929167df531eb2da00063a7d", false, + }, + { + "Missing Date header", + map[string]interface{}{"X-Signature": "999395e38ac48988929167df531eb2da00063a7d"}, + []byte(`{"a": "b"}`), "bilFGi4ZVZUdG+C6r0NIM9tuRq6PaG33R3eBUVhLwMAErGBaazvXe4Gq2DcJs5q+", + "48e395e38ac48988929167df531eb2da00063a7d", false, + }, + { + "Missing X-Signature header", + map[string]interface{}{"Date": "Thu 07 Sep 2017 06:30:04 UTC"}, + []byte(`{"a": "b"}`), "bilFGi4ZVZUdG+C6r0NIM9tuRq6PaG33R3eBUVhLwMAErGBaazvXe4Gq2DcJs5q+", + "48e395e38ac48988929167df531eb2da00063a7d", false, + }, +} + +func TestCheckScalrSignature(t *testing.T) { + for _, testCase := range checkScalrSignatureTests { + valid, err := CheckScalrSignature(testCase.headers, testCase.payload, testCase.secret, false) + if valid != testCase.ok { + t.Errorf("failed to check scalr signature fot test case: %s\nexpected ok:%#v, got ok:%#v}", + testCase.description, testCase.ok, valid) + } + + if err != nil && strings.Contains(err.Error(), testCase.expectedSignature) { + t.Errorf("error message should not disclose expected mac: %s on test case %s", err, testCase.description) + } + } } var extractParameterTests = []struct { diff --git a/test/hookecho.go b/test/hookecho.go index 8e308ff..6e5e9f7 100644 --- a/test/hookecho.go +++ b/test/hookecho.go @@ -5,8 +5,8 @@ package main import ( "fmt" "os" - "strings" "strconv" + "strings" ) func main() { From 7da4d8ba9fafe7cc779a47f7b4d6c5c1d2151ff0 Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Fri, 16 Feb 2018 20:31:23 -0600 Subject: [PATCH 13/15] Use strings.Contains --- hook/hook.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hook/hook.go b/hook/hook.go index f5e20ad..79f624a 100644 --- a/hook/hook.go +++ b/hook/hook.go @@ -196,7 +196,7 @@ func CheckIPWhitelist(remoteAddr string, ipRange string) (bool, error) { ipRange = strings.TrimSpace(ipRange) - if strings.Index(ipRange, "/") == -1 { + if !strings.Contains(ipRange, "/") { ipRange = ipRange + "/32" } From d85ee5e068ef598e5f9bd61f16b24a7f018f046f Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Fri, 16 Feb 2018 20:33:17 -0600 Subject: [PATCH 14/15] Use strings.TrimPrefix --- hook/hook.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/hook/hook.go b/hook/hook.go index 79f624a..5f5b3de 100644 --- a/hook/hook.go +++ b/hook/hook.go @@ -94,9 +94,7 @@ func (e *ParseError) Error() string { // CheckPayloadSignature calculates and verifies SHA1 signature of the given payload func CheckPayloadSignature(payload []byte, secret string, signature string) (string, error) { - if strings.HasPrefix(signature, "sha1=") { - signature = signature[5:] - } + signature = strings.TrimPrefix(signature, "sha1=") mac := hmac.New(sha1.New, []byte(secret)) _, err := mac.Write(payload) @@ -113,9 +111,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) { - if strings.HasPrefix(signature, "sha256=") { - signature = signature[7:] - } + signature = strings.TrimPrefix(signature, "sha256=") mac := hmac.New(sha256.New, []byte(secret)) _, err := mac.Write(payload) From 66a9e48e3914ea590feb39660fe68aedbaca9d0b Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Fri, 16 Feb 2018 20:36:42 -0600 Subject: [PATCH 15/15] Fix unnecessary nil check around range --- webhook.go | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/webhook.go b/webhook.go index 3014b1c..2ca49e0 100644 --- a/webhook.go +++ b/webhook.go @@ -250,10 +250,9 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { } // handle hook - if errors := matchedHook.ParseJSONParameters(&headers, &query, &payload); errors != nil { - for _, err := range errors { - log.Printf("[%s] error parsing JSON parameters: %s\n", rid, err) - } + errors := matchedHook.ParseJSONParameters(&headers, &query, &payload) + for _, err := range errors { + log.Printf("[%s] error parsing JSON parameters: %s\n", rid, err) } var ok bool @@ -341,27 +340,21 @@ func handleHook(h *hook.Hook, rid string, headers, query, payload *map[string]in cmd.Dir = h.CommandWorkingDirectory cmd.Args, errors = h.ExtractCommandArguments(headers, query, payload) - if errors != nil { - for _, err := range errors { - log.Printf("[%s] error extracting command arguments: %s\n", rid, err) - } + for _, err := range errors { + log.Printf("[%s] error extracting command arguments: %s\n", rid, err) } var envs []string envs, errors = h.ExtractCommandArgumentsForEnv(headers, query, payload) - if errors != nil { - for _, err := range errors { - log.Printf("[%s] error extracting command arguments for environment: %s\n", rid, err) - } + for _, err := range errors { + log.Printf("[%s] error extracting command arguments for environment: %s\n", rid, err) } files, errors := h.ExtractCommandArgumentsForFile(headers, query, payload) - if errors != nil { - for _, err := range errors { - log.Printf("[%s] error extracting command arguments for file: %s\n", rid, err) - } + for _, err := range errors { + log.Printf("[%s] error extracting command arguments for file: %s\n", rid, err) } for i := range files {