From 1a17dc83fe580557d0065e981fa8d480a8c65de5 Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Wed, 2 Jan 2019 16:09:27 -0600 Subject: [PATCH] Return errors on empty secrets during signature validations Fixes #207 --- hook/hook.go | 12 ++++++++++++ hook/hook_test.go | 14 +++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/hook/hook.go b/hook/hook.go index d8e4635..ef03e0b 100644 --- a/hook/hook.go +++ b/hook/hook.go @@ -94,6 +94,10 @@ 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 secret == "" { + return "", errors.New("signature validation secret can not be empty") + } + signature = strings.TrimPrefix(signature, "sha1=") mac := hmac.New(sha1.New, []byte(secret)) @@ -111,6 +115,10 @@ 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 secret == "" { + return "", errors.New("signature validation secret can not be empty") + } + signature = strings.TrimPrefix(signature, "sha256=") mac := hmac.New(sha256.New, []byte(secret)) @@ -134,6 +142,10 @@ func CheckScalrSignature(headers map[string]interface{}, body []byte, signingKey if _, ok := headers["Date"]; !ok { return false, nil } + if signingKey == "" { + return false, errors.New("signature validation signing key can not be empty") + } + providedSignature := headers["X-Signature"].(string) dateHeader := headers["Date"].(string) mac := hmac.New(sha1.New, []byte(signingKey)) diff --git a/hook/hook_test.go b/hook/hook_test.go index 1794a7c..bba9136 100644 --- a/hook/hook_test.go +++ b/hook/hook_test.go @@ -19,6 +19,7 @@ var checkPayloadSignatureTests = []struct { // failures {[]byte(`{"a": "z"}`), "secret", "XXXe04cbb22afa8ffbff8796fc1894ed27badd9e", "b17e04cbb22afa8ffbff8796fc1894ed27badd9e", false}, {[]byte(`{"a": "z"}`), "secreX", "b17e04cbb22afa8ffbff8796fc1894ed27badd9e", "900225703e9342328db7307692736e2f7cc7b36e", false}, + {[]byte(`{"a": "z"}`), "", "b17e04cbb22afa8ffbff8796fc1894ed27badd9e", "", false}, } func TestCheckPayloadSignature(t *testing.T) { @@ -28,7 +29,7 @@ func TestCheckPayloadSignature(t *testing.T) { t.Errorf("failed to check payload signature {%q, %q, %q}:\nexpected {mac:%#v, ok:%#v},\ngot {mac:%#v, ok:%#v}", tt.payload, tt.secret, tt.signature, tt.mac, tt.ok, mac, (err == nil)) } - if err != nil && strings.Contains(err.Error(), tt.mac) { + if err != nil && tt.mac != "" && strings.Contains(err.Error(), tt.mac) { t.Errorf("error message should not disclose expected mac: %s", err) } } @@ -45,6 +46,7 @@ var checkPayloadSignature256Tests = []struct { {[]byte(`{"a": "z"}`), "secret", "sha256=f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", "f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", true}, // failures {[]byte(`{"a": "z"}`), "secret", "XXX7af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", "f417af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", false}, + {[]byte(`{"a": "z"}`), "", "XXX7af3a21bd70379b5796d5f013915e7029f62c580fb0f500f59a35a6f04c89", "", false}, } func TestCheckPayloadSignature256(t *testing.T) { @@ -54,7 +56,7 @@ func TestCheckPayloadSignature256(t *testing.T) { t.Errorf("failed to check payload signature {%q, %q, %q}:\nexpected {mac:%#v, ok:%#v},\ngot {mac:%#v, ok:%#v}", tt.payload, tt.secret, tt.signature, tt.mac, tt.ok, mac, (err == nil)) } - if err != nil && strings.Contains(err.Error(), tt.mac) { + if err != nil && tt.mac != "" && strings.Contains(err.Error(), tt.mac) { t.Errorf("error message should not disclose expected mac: %s", err) } } @@ -92,6 +94,12 @@ var checkScalrSignatureTests = []struct { []byte(`{"a": "b"}`), "bilFGi4ZVZUdG+C6r0NIM9tuRq6PaG33R3eBUVhLwMAErGBaazvXe4Gq2DcJs5q+", "48e395e38ac48988929167df531eb2da00063a7d", false, }, + { + "Missing signing key", + map[string]interface{}{"Date": "Thu 07 Sep 2017 06:30:04 UTC", "X-Signature": "48e395e38ac48988929167df531eb2da00063a7d"}, + []byte(`{"a": "b"}`), "", + "48e395e38ac48988929167df531eb2da00063a7d", false, + }, } func TestCheckScalrSignature(t *testing.T) { @@ -102,7 +110,7 @@ func TestCheckScalrSignature(t *testing.T) { testCase.description, testCase.ok, valid) } - if err != nil && strings.Contains(err.Error(), testCase.expectedSignature) { + if err != nil && testCase.secret != "" && strings.Contains(err.Error(), testCase.expectedSignature) { t.Errorf("error message should not disclose expected mac: %s on test case %s", err, testCase.description) } }