From 7fa3a8900c6a34a2874acb770bf1022a70a77bfa Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Mon, 30 Dec 2019 21:51:11 -0600 Subject: [PATCH] Show failed parameter node lookups When attempting to match a JSON path for initial setup, it would be helpful to know where the path failed. This change logs the failed parameter node. For example, if you are trying to match path "a.b.d.e", but you failed to include the "c" node, webhook will log an error "parameter node not found: d.e" to assist in troubleshooting. --- internal/hook/hook.go | 152 ++++++++++++++++++++++--------------- internal/hook/hook_test.go | 26 +++---- webhook.go | 14 ++-- 3 files changed, 113 insertions(+), 79 deletions(-) diff --git a/internal/hook/hook.go b/internal/hook/hook.go index 1131eb4..2ec3525 100644 --- a/internal/hook/hook.go +++ b/internal/hook/hook.go @@ -46,6 +46,28 @@ const ( EnvNamespace string = "HOOK_" ) +// ParameterNodeError describes an error walking a parameter node. +type ParameterNodeError struct { + key string +} + +func (e *ParameterNodeError) Error() string { + if e == nil { + return "" + } + return fmt.Sprintf("parameter node not found: %s", e.key) +} + +// IsParameterNodeError returns whether err is of type ParameterNodeError. +func IsParameterNodeError(err error) bool { + switch err.(type) { + case *ParameterNodeError: + return true + default: + return false + } +} + // SignatureError describes an invalid payload signature passed to Hook. type SignatureError struct { Signature string @@ -274,9 +296,9 @@ func ReplaceParameter(s string, params interface{}, value interface{}) bool { } // GetParameter extracts interface{} value based on the passed string -func GetParameter(s string, params interface{}) (interface{}, bool) { +func GetParameter(s string, params interface{}) (interface{}, error) { if params == nil { - return nil, false + return nil, errors.New("no parameters") } paramsValue := reflect.ValueOf(params) @@ -290,7 +312,7 @@ func GetParameter(s string, params interface{}) (interface{}, bool) { index, err := strconv.ParseUint(p[0], 10, 64) if err != nil || paramsValueSliceLength <= int(index) { - return nil, false + return nil, &ParameterNodeError{s} } return GetParameter(p[1], params.([]interface{})[index]) @@ -299,18 +321,18 @@ func GetParameter(s string, params interface{}) (interface{}, bool) { index, err := strconv.ParseUint(s, 10, 64) if err != nil || paramsValueSliceLength <= int(index) { - return nil, false + return nil, &ParameterNodeError{s} } - return params.([]interface{})[index], true + return params.([]interface{})[index], nil } - return nil, false + return nil, &ParameterNodeError{s} case reflect.Map: // Check for raw key if v, ok := params.(map[string]interface{})[s]; ok { - return v, true + return v, nil } // Checked for dotted references @@ -320,19 +342,21 @@ func GetParameter(s string, params interface{}) (interface{}, bool) { return GetParameter(p[1], pValue) } - return pValue, true + return pValue, nil } } - return nil, false + return nil, &ParameterNodeError{s} } // ExtractParameterAsString extracts value from interface{} as string based on the passed string -func ExtractParameterAsString(s string, params interface{}) (string, bool) { - if pValue, ok := GetParameter(s, params); ok { - return fmt.Sprintf("%v", pValue), true +func ExtractParameterAsString(s string, params interface{}) (string, error) { + pValue, err := GetParameter(s, params) + if err != nil { + return "", err } - return "", false + + return fmt.Sprintf("%v", pValue), nil } // Argument type specifies the parameter key name and the source it should @@ -346,7 +370,7 @@ type Argument struct { // Get Argument method returns the value for the Argument's key name // based on the Argument's source -func (ha *Argument) Get(headers, query, payload *map[string]interface{}) (string, bool) { +func (ha *Argument) Get(headers, query, payload *map[string]interface{}) (string, error) { var source *map[string]interface{} key := ha.Name @@ -359,35 +383,35 @@ func (ha *Argument) Get(headers, query, payload *map[string]interface{}) (string case SourcePayload: source = payload case SourceString: - return ha.Name, true + return ha.Name, nil case SourceEntirePayload: r, err := json.Marshal(payload) if err != nil { - return "", false + return "", err } - return string(r), true + return string(r), nil case SourceEntireHeaders: r, err := json.Marshal(headers) if err != nil { - return "", false + return "", err } - return string(r), true + return string(r), nil case SourceEntireQuery: r, err := json.Marshal(query) if err != nil { - return "", false + return "", err } - return string(r), true + return string(r), nil } if source != nil { return ExtractParameterAsString(key, *source) } - return "", false + return "", errors.New("no source for value retrieval") } // Header is a structure containing header name and it's value @@ -469,7 +493,10 @@ func (h *Hook) ParseJSONParameters(headers, query, payload *map[string]interface errors := make([]error, 0) for i := range h.JSONStringParameters { - if arg, ok := h.JSONStringParameters[i].Get(headers, query, payload); ok { + arg, err := h.JSONStringParameters[i].Get(headers, query, payload) + if err != nil { + errors = append(errors, &ArgumentError{h.JSONStringParameters[i]}) + } else { var newArg map[string]interface{} decoder := json.NewDecoder(strings.NewReader(string(arg))) @@ -503,8 +530,6 @@ func (h *Hook) ParseJSONParameters(headers, query, payload *map[string]interface } else { errors = append(errors, &SourceError{h.JSONStringParameters[i]}) } - } else { - errors = append(errors, &ArgumentError{h.JSONStringParameters[i]}) } } @@ -524,12 +549,14 @@ func (h *Hook) ExtractCommandArguments(headers, query, payload *map[string]inter args = append(args, h.ExecuteCommand) for i := range h.PassArgumentsToCommand { - if arg, ok := h.PassArgumentsToCommand[i].Get(headers, query, payload); ok { - args = append(args, arg) - } else { + arg, err := h.PassArgumentsToCommand[i].Get(headers, query, payload) + if err != nil { args = append(args, "") errors = append(errors, &ArgumentError{h.PassArgumentsToCommand[i]}) + continue } + + args = append(args, arg) } if len(errors) > 0 { @@ -546,16 +573,18 @@ func (h *Hook) ExtractCommandArgumentsForEnv(headers, query, payload *map[string args := make([]string, 0) errors := make([]error, 0) for i := range h.PassEnvironmentToCommand { - if arg, ok := h.PassEnvironmentToCommand[i].Get(headers, query, payload); ok { - if h.PassEnvironmentToCommand[i].EnvName != "" { - // first try to use the EnvName if specified - args = append(args, h.PassEnvironmentToCommand[i].EnvName+"="+arg) - } else { - // then fallback on the name - args = append(args, EnvNamespace+h.PassEnvironmentToCommand[i].Name+"="+arg) - } - } else { + arg, err := h.PassEnvironmentToCommand[i].Get(headers, query, payload) + if err != nil { errors = append(errors, &ArgumentError{h.PassEnvironmentToCommand[i]}) + continue + } + + if h.PassEnvironmentToCommand[i].EnvName != "" { + // first try to use the EnvName if specified + args = append(args, h.PassEnvironmentToCommand[i].EnvName+"="+arg) + } else { + // then fallback on the name + args = append(args, EnvNamespace+h.PassEnvironmentToCommand[i].Name+"="+arg) } } @@ -580,30 +609,30 @@ func (h *Hook) ExtractCommandArgumentsForFile(headers, query, payload *map[strin args := make([]FileParameter, 0) errors := make([]error, 0) for i := range h.PassFileToCommand { - if arg, ok := h.PassFileToCommand[i].Get(headers, query, payload); ok { - - if h.PassFileToCommand[i].EnvName == "" { - // if no environment-variable name is set, fall-back on the name - log.Printf("no ENVVAR name specified, falling back to [%s]", EnvNamespace+strings.ToUpper(h.PassFileToCommand[i].Name)) - h.PassFileToCommand[i].EnvName = EnvNamespace + strings.ToUpper(h.PassFileToCommand[i].Name) - } - - var fileContent []byte - if h.PassFileToCommand[i].Base64Decode { - dec, err := base64.StdEncoding.DecodeString(arg) - if err != nil { - log.Printf("error decoding string [%s]", err) - } - fileContent = []byte(dec) - } else { - fileContent = []byte(arg) - } - - args = append(args, FileParameter{EnvName: h.PassFileToCommand[i].EnvName, Data: fileContent}) - - } else { + arg, err := h.PassFileToCommand[i].Get(headers, query, payload) + if err != nil { errors = append(errors, &ArgumentError{h.PassFileToCommand[i]}) + continue } + + if h.PassFileToCommand[i].EnvName == "" { + // if no environment-variable name is set, fall-back on the name + log.Printf("no ENVVAR name specified, falling back to [%s]", EnvNamespace+strings.ToUpper(h.PassFileToCommand[i].Name)) + h.PassFileToCommand[i].EnvName = EnvNamespace + strings.ToUpper(h.PassFileToCommand[i].Name) + } + + var fileContent []byte + if h.PassFileToCommand[i].Base64Decode { + dec, err := base64.StdEncoding.DecodeString(arg) + if err != nil { + log.Printf("error decoding string [%s]", err) + } + fileContent = []byte(dec) + } else { + fileContent = []byte(arg) + } + + args = append(args, FileParameter{EnvName: h.PassFileToCommand[i].EnvName, Data: fileContent}) } if len(errors) > 0 { @@ -785,7 +814,8 @@ func (r MatchRule) Evaluate(headers, query, payload *map[string]interface{}, bod return CheckScalrSignature(*headers, *body, r.Secret, true) } - if arg, ok := r.Parameter.Get(headers, query, payload); ok { + arg, err := r.Parameter.Get(headers, query, payload) + if err == nil { switch r.Type { case MatchValue: return compare(arg, r.Value), nil @@ -802,7 +832,7 @@ func (r MatchRule) Evaluate(headers, query, payload *map[string]interface{}, bod return err == nil, err } } - return false, nil + return false, err } // compare is a helper function for constant time string comparisons. diff --git a/internal/hook/hook_test.go b/internal/hook/hook_test.go index dfb4874..78319a4 100644 --- a/internal/hook/hook_test.go +++ b/internal/hook/hook_test.go @@ -28,9 +28,9 @@ func TestGetParameter(t *testing.T) { {"z.b", map[string]interface{}{"a": map[string]interface{}{"z": 2}}, nil, false}, {"a.2", map[string]interface{}{"a": []interface{}{"a", "b"}}, nil, false}, } { - res, ok := GetParameter(test.key, test.val) - if ok != test.ok { - t.Errorf("unexpected result given {%q, %q}: %t\n", test.key, test.val, ok) + res, err := GetParameter(test.key, test.val) + if (err == nil) != test.ok { + t.Errorf("unexpected result given {%q, %q}: %s\n", test.key, test.val, err) } if !reflect.DeepEqual(res, test.expect) { @@ -225,9 +225,9 @@ var extractParameterTests = []struct { func TestExtractParameter(t *testing.T) { for _, tt := range extractParameterTests { - value, ok := ExtractParameterAsString(tt.s, tt.params) - if ok != tt.ok || value != tt.value { - t.Errorf("failed to extract parameter %q:\nexpected {value:%#v, ok:%#v},\ngot {value:%#v, ok:%#v}", tt.s, tt.value, tt.ok, value, ok) + value, err := ExtractParameterAsString(tt.s, tt.params) + if (err == nil) != tt.ok || value != tt.value { + t.Errorf("failed to extract parameter %q:\nexpected {value:%#v, ok:%#v},\ngot {value:%#v, err:%s}", tt.s, tt.value, tt.ok, value, err) } } } @@ -252,9 +252,9 @@ var argumentGetTests = []struct { func TestArgumentGet(t *testing.T) { for _, tt := range argumentGetTests { a := Argument{tt.source, tt.name, "", false} - value, ok := a.Get(tt.headers, tt.query, tt.payload) - if ok != tt.ok || value != tt.value { - t.Errorf("failed to get {%q, %q}:\nexpected {value:%#v, ok:%#v},\ngot {value:%#v, ok:%#v}", tt.source, tt.name, tt.value, tt.ok, value, ok) + value, err := a.Get(tt.headers, tt.query, tt.payload) + if (err == nil) != tt.ok || value != tt.value { + t.Errorf("failed to get {%q, %q}:\nexpected {value:%#v, ok:%#v},\ngot {value:%#v, err:%s}", tt.source, tt.name, tt.value, tt.ok, value, err) } } } @@ -452,7 +452,7 @@ var matchRuleTests = []struct { // failures {"value", "", "", "X", "", Argument{"header", "a", "", false}, &map[string]interface{}{"A": "z"}, nil, nil, []byte{}, "", false, false}, {"regex", "^X", "", "", "", Argument{"header", "a", "", false}, &map[string]interface{}{"A": "z"}, nil, nil, []byte{}, "", false, false}, - {"value", "", "2", "X", "", Argument{"header", "a", "", false}, &map[string]interface{}{"Y": "z"}, nil, nil, []byte{}, "", false, false}, // reference invalid header + {"value", "", "2", "X", "", Argument{"header", "a", "", false}, &map[string]interface{}{"Y": "z"}, nil, nil, []byte{}, "", false, true}, // reference invalid header // errors {"regex", "*", "", "", "", Argument{"header", "a", "", false}, &map[string]interface{}{"A": "z"}, nil, nil, []byte{}, "", false, true}, // invalid regex {"payload-hash-sha1", "", "secret", "", "", Argument{"header", "a", "", false}, &map[string]interface{}{"A": ""}, nil, nil, []byte{}, "", false, true}, // invalid hmac @@ -477,7 +477,7 @@ func TestMatchRule(t *testing.T) { r := MatchRule{tt.typ, tt.regex, tt.secret, tt.value, tt.param, tt.ipRange} ok, err := r.Evaluate(tt.headers, tt.query, tt.payload, &tt.body, tt.remoteAddr) if ok != tt.ok || (err != nil) != tt.err { - t.Errorf("%d failed to match %#v:\nexpected ok: %#v, err: %v\ngot ok: %#v, err: %v", i, r, tt.ok, tt.err, ok, (err != nil)) + t.Errorf("%d failed to match %#v:\nexpected ok: %#v, err: %v\ngot ok: %#v, err: %v", i, r, tt.ok, tt.err, ok, err) } } } @@ -540,7 +540,7 @@ var andRuleTests = []struct { "invalid rule", AndRule{{Match: &MatchRule{"value", "", "", "X", Argument{"header", "a", "", false}, ""}}}, &map[string]interface{}{"Y": "z"}, nil, nil, nil, - false, false, + false, true, }, } @@ -595,7 +595,7 @@ var orRuleTests = []struct { {Match: &MatchRule{"value", "", "", "z", Argument{"header", "a", "", false}, ""}}, }, &map[string]interface{}{"Y": "Z"}, nil, nil, []byte{}, - false, false, + false, true, }, } diff --git a/webhook.go b/webhook.go index a502a34..ee9cd20 100644 --- a/webhook.go +++ b/webhook.go @@ -473,11 +473,15 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { } else { 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.Println(msg) - w.WriteHeader(http.StatusInternalServerError) - fmt.Fprint(w, "Error occurred while evaluating hook rules.") - return + if !hook.IsParameterNodeError(err) { + msg := fmt.Sprintf("[%s] error evaluating hook: %s", rid, err) + log.Println(msg) + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, "Error occurred while evaluating hook rules.") + return + } + + log.Printf("[%s] %v", rid, err) } }