Bug 1213630

Summary: Webhook header needs to include cryptographic signature in header for identification.
Product: [Retired] Zanata Reporter: Alex Eng <aeng>
Component: SecurityAssignee: Alex Eng <aeng>
Status: CLOSED CURRENTRELEASE QA Contact: Damian Jansen <djansen>
Severity: high Docs Contact:
Priority: high    
Version: 3.7CC: camunoz, dchen, djansen, mkim, rbean, sflaniga, zanata-bugs
Target Milestone: ---   
Target Release: 3.8   
Hardware: Unspecified   
OS: Unspecified   
Fixed In Version: 3.8.0-SNAPSHOT (git-jenkins-zanata-server-github-pull-requests-3971) Doc Type: Bug Fix
Doc Text:
Story Points: 2
Clone Of: Environment:
Last Closed: 2016-01-21 01:40:14 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:

Description Alex Eng 2015-04-21 00:19:04 UTC
Description of problem:

Current Webhook post data doesn't have any Zanata identification. This would potentially expose the listener of Zanata webhook to any anonymous POST. 

Zanata webhook should include cryptographic signature for security purposes.

Comment 1 Ralph Bean 2015-06-18 19:06:24 UTC
Just to clarify, we can't actually use the webhooks feature in Fedora Infrastructure unless this is implemented.

If anyone used the webhooks feature, there is no way currently to verify that the POST actually comes from zanata.org, and so they would be wide open to abuse.

It would be quite convenient if zanata signed their messages in the same way or in a similar way that github.com does.


Comment 2 Alex Eng 2015-06-22 03:49:20 UTC
This bug should also make changes to the json output
- include URL for the related document in project-version

Current output:
    "eventType": "org.zanata.events.DocumentMilestoneEvent",
    "milestone": "100% Translated",
    "locale": "as",
    "docId": "anaconda",
    "version": "copy-7",
    "project": "anaconda"

Comment 3 David Mason 2015-07-08 04:03:50 UTC
 - Add a field in project -> settings -> webhooks for an *optional* secret key
 - Hash message body using sha1/HMAC using the secret key
   - will need to generate the JSON up-front to sign it
 - Send the hash with the webhook response in header "X-Zanata-Signature".
 - Do not include "X-Zanata-Signature" header if no secret key has been specified for the project.

Note: make sure we are setting the content type header to JSON.

Technical note: may be able to use some kind of RestEasy interceptor, or generate the body ourselves to have access to it.

Comment 4 Sean Flanigan 2015-07-09 01:34:14 UTC
Since our header doesn't have any historical baggage yet, rather than duplicate GitHub's header exactly, I suggest we make it a little stronger, and very similar to Trello.

According to the GitHub docs, use of the GitHub header may be vulnerable to a timing attack if the webhook receiver fails to use a constant-time string comparison.  Also, I think it may be vulnerable to another attack, if the same secret is used for multiple webhook URLs (replaying a webhook for one URL against the other).

The Trello header uses a double HMAC (to defeat the timing attack), plus the hash includes both the body and the URL (to defeat the replay attack).

Trello docs and sample implementation: https://trello.com/docs/gettingstarted/webhooks.html#triggering-webhooks

The timing attack: http://web.archive.org/web/20141016010907/https://www.isecpartners.com/blog/2011/february/double-hmac-verification.aspx

There are implementations which go further (including some of the HTTP headers in the hash), but I don't want to add too much to the complexity

Instead of "X-Zanata-Signature" (what sort of signature?), I suggest the header name "X-Zanata-Webhook".

I don't think we clarified this before, but I recommend we use Base64 (like Trello), not hexadecimal (like GitHub).

Ralph, would you say including the URL in the hash will make signature verification too complex at the receiver's end?

Comment 5 Ralph Bean 2015-07-09 14:52:05 UTC
No, that sounds fine.  It's not too much to ask the recipient to know where they live.  Thanks for the additions Sean.

Comment 6 Alex Eng 2015-07-10 05:37:59 UTC
Pull request:

Comment 7 Damian Jansen 2015-07-16 01:26:20 UTC
Verified at 52366c98ca6e423cafad8d363d751fce4442b1a9

Comment 8 Damian Jansen 2015-07-21 04:08:12 UTC
Verified merge bd33f08c933c8572725d0d8f131cd654aaee5b50

Comment 9 Michelle Kim 2015-07-22 00:16:13 UTC
Documentation link: