redmine_webhook icon indicating copy to clipboard operation
redmine_webhook copied to clipboard

Added HMAC signature to webhook payloads

Open ricekab opened this issue 3 years ago • 1 comments

This implemented the feature requested in #9 .

Changes

  • NEW: "secret_key" text field in Webhook model.
  • NEW: Webhook view & controller modified to include secret_key text field (tagged as password field in the view).
  • NEW: Webhook posts contain two new headers.
    • X-RedmineWebhook-HMAC-Alg: The algorithm used for the HMAC signature. Currently hard-coded as sha1.
    • X-RedmineWebhook-HMAC-Signature: The HMAC signature.

Testing

The changes were tested against the latest docker redmine container (5.0.3 at the time of this PR). A small Python Flask server was written to perform the HMAC validation that clients are expected to do (can be found in this gist).

I've tested with three configurations: (1) an incorrect key, (2) a correct key, and (3) no key. image

The Flask web server output for this is:

Project: examplebadkey
Alg: sha1
Signature: 89b3f8de7175044d4f772ec3ce7b6741aa852431
Calculated signature: b9ad1a2263b192d7d8978d13f0cb8d8ed68175d8
HMAC verification failed, payload is malformed or tampered!
HMAC verification succeeded: False
172.18.0.2 - - [23/Oct/2022 13:42:28] "POST /redminewebhook/examplebadkey HTTP/1.1" 200 -

Project: goodkey
Alg: sha1
Signature: b9ad1a2263b192d7d8978d13f0cb8d8ed68175d8
Calculated signature: b9ad1a2263b192d7d8978d13f0cb8d8ed68175d8
HMAC verification succeeded: True
172.18.0.2 - - [23/Oct/2022 13:42:28] "POST /redminewebhook/goodkey HTTP/1.1" 200 -

Project: nokey
Alg: sha1
Signature: 2880f307aa1ba774c716279b26542d6011e5fe6b
Calculated signature: b9ad1a2263b192d7d8978d13f0cb8d8ed68175d8
HMAC verification failed, payload is malformed or tampered!
HMAC verification succeeded: False
172.18.0.2 - - [23/Oct/2022 13:42:28] "POST /redminewebhook/nokey HTTP/1.1" 200 -

Potential issues / improvements

  • The algorithm used for the digest is hard-coded to use SHA1. This should be configurable to some extent.
  • This is potentially a breaking change with existing webhook setups. I'm not sure how database migrations work with existing entries. If the secret_key is not a proper text entry (ie. null), I expect this code could break.

Additional notes

  • I followed the naming convention (somewhat) for db migrations from the main redmine project, prepending the date at the front in YYYYMMDD format.
  • I do not have much experience with Ruby development, verification from a maintainer regarding code quality would be much appreciated.

ricekab avatar Oct 23 '22 14:10 ricekab

I just realized this may require a version increment potentially? Let me know if that's required (or add it on top).

ricekab avatar Oct 23 '22 14:10 ricekab