Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added HMAC signature to webhook payloads #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ricekab
Copy link

@ricekab ricekab commented Oct 23, 2022

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.

…erification

signature.

Currently this is hard-coded to use sha1, this should be made configurable on a global or project basis. (Or perhaps even per hook?).

Direct link to upstream issue: suer#9
@ricekab
Copy link
Author

ricekab commented Oct 23, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant