Proposal: more flexible argument extraction and signature testing

[ianroberts]

This started out as a more general fix for #643 but it has evolved into a larger refactor with a number of open questions, so I'm submitting it as a draft PR initially for discussion:

Overview

Issue #643 discusses the idea of verifying a signature that was computed over something other than just the request body content, and the related PR #644 provides an implementation of one special case of this general idea, concatenating one or more request header values to the end of the body data before calculating the signature. However there are many variations on this theme, for example

• Drone.io global webhook signature #455 talks about validating (rather complex) signatures over a string built up from several different request headers
• Add support for validating MS Teams outgoing webhooks #512 the HMAC is computed over the body data but the signature value that you compare with has to be extracted as a substring of one of the request headers, not the whole header value.
• Concatenate parameters in command line #336 and Combine multiple JSON parameters to one argument? #601 are not about signatures but still ask for a way to combine different parts of the payload into a single argument extraction

In this PR I've tried to come up with a general way to support cases like those as well.

The implementation is in two parts:

1. I've introduced a new Argument type "source": "template", where the argument value is computed by evaluating a Go template against a context derived from the Request. This lets you combine values from different headers, take substrings, etc.
2. I've refactored the various payload-hmac-* "match" rules into a new "check-signature" rule type, which gives the option to use an Argument for the "string-to-sign" as well as the one for the reference signature value.

Together these let you compute signatures over any combination of the body, headers and query parameters, concatenated together with no delimiter, newlines, colons, etc. as required by your particular webhook sender.

Template argument type

- source: template
  name: |
    {{- .BodyText -}}:{{- .GetHeader "x-request-id" -}}

The template argument type evaluates a Go template to build the argument value. The template context gives access to all the things that the other argument types provide, including

• the raw payload (.Body as a []byte, .BodyText as a string)
• the parsed JSON/XML payload (.Payload)
• URL query parameters (.Query)
• request headers (the raw map as .Headers, but also the .GetHeader "name" function in the example above that canonicalises the header name the same way as a "source": "header" argument would do)
• webhook's own request ID (.ID)
• the remote address, content type and request method (.RemoteAddr, .ContentType and .Method)

I've deliberately not exposed the whole of the RawRequest from the internal Request struct.

The check-signature rule type

I've refactored all the payload-hmac-sha1/256/512 match types into a new kind of rule. The old

{
  "match":
  {
    "type": "payload-hmac-<type>",
    "secret": "secret",
    "parameter":
    {
      "source": "header",
      "name": "X-Signature"
    }
  }
}

is now expressed as

{
  "check-signature":
  {
    "algorithm": "<type>",
    "secret": "secret",
    "signature":
    {
      "source": "header",
      "name": "X-Signature"
    }
  }
}

However, as well as "algorithm", "secret" and "signature", you can now specify "string-to-sign" as another argument, which could be a simple argument like a single header

{
  "check-signature":
  { "algorithm": "sha256", "secret": "secret",
    "signature":
    {
      "source": "header",
      "name": "X-Signature"
    },
    "string-to-sign":
    {
      "source": "header",
      "name": "Digest"
    }
  }
}

or it could be a template argument like the example above

- check-signature:
    algorithm: sha256
    secret: my-secret
    signature:
      source: header
      name: x-signature
    string-to-sign:
      source: template
        name: |
          {{- .BodyText -}}:{{- .GetHeader "x-request-id" -}}

Backwards compatibility

I've made sure that existing hooks files (YAML and JSON) will still parse as before, with the payload-hmac-* match rules being converted to check-signature rules during the loading process. The only wrinkle is if you want to start using templates in arguments and you need to parse the hooks file itself as a template, then it can get quite fiddly to escape the runtime templates from the parse-time engine. To mitigate this I've added a -template-delims CLI option to change the delimiters used for the parse-time template engine, so you can say -template-delims='[[,]]' and then use [[ getenv "..." ]] for the parse-time template and {{ .GetHeader "..." }} for the runtime ones.

---

Open questions for discussion

Are Go Templates the right paradigm?

Go templates are very flexible, but should we stick with something simpler and with fewer gotchas around whitespace handling etc.? #512 mentions cel-go as an alternative.

Template context

Is the template context appropriate? Are there other things we could/should expose to the templates, or more sensible names for the things that are already there?

Do we want to add some more useful functions to the template engine? One extreme would be to introduce something like Sprig, the same function library used by Helm. This includes functions for things like date parsing and formatting, which would also open up a way to use template arguments to check things like "is the Date header less than 15 minutes old":

- match:
    type: "value"
    value: "true"
    parameter:
      source: template
      name: |
        {{- (.GetHeader "date" | toDate "Mon Jan 2 15:04:05 MST 2006").After (now | dateModify "-15m") -}}

Though something like that probably deserves its own dedicated rule type.

[moorereason]

I haven't had much time to commit to webhook in a while, but this PR got my attention. Kudos for making a great PR and explaining the situation well.

Regarding templates, see this discussion. I feel like HCL is probably too complex and may hem us in in some areas. For purely adding a template language to our existing configs, I'd vote for expr. See what you think about it.

I'm curious if we could solve the various hmac scenarios with templates instead of extending the config schema with the check-signature and string-to-sign stuff. I like the simplicity of the existing schema and am not against extending it to make it easier on the end users, but some of these hmac requirements are pretty complex. I'd recommend thinking through the template feature more to see how far it can take us.

[moorereason]

Iterating on this idea using expr while trying to stick with the match model:

# Assumes the hmac() function could derive the hash function and secret from the match stanza.
- match:
    type: hmac-sha256
    secret: secret
    parameter:
      source: expr
      name: |
        hmac($bodyText + ":" + getHeader("x-request-id")) == trimPrefix(getHeader("x-signature"), "sha256:")

- match:
    type: value
    value: true
    parameter:
      source: expr
        name: |
          date(getHeader("date")) > now() - duration("15m")

[adnanh]

I've been wanting to implement this for a while now, but never got the necessary focus time...

I agree with @moorereason that some generic expressions format would be a more human readable format compared to Go templates, but either way it would be a long awaited huuuge improvement to webhook.

At my full time job, I used to work a lot with mapbox mapping SDK which includes expressions for data styling, the bottom line is that it was very simple to learn and very powerful to use. Here's a documentation page for those, maybe it could serve as a source of inspiration for this.

[ianroberts]

Thanks for the feedback. I haven't forgotten about this - I do like the look of expr and I intend to see if I can re-work this PR to use that instead of text/template, but this is a personal project rather than a work-related one so I may not have time to do it properly until nearer Christmas.

[adnanh]

Thanks for the feedback. I haven't forgotten about this - I do like the look of expr and I intend to see if I can re-work this PR to use that instead of text/template, but this is a personal project rather than a work-related one so I may not have time to do it properly until nearer Christmas.

Of course :-) Happy holidays!