Add X-Hub-Signature header to webhook deliveries #16115
Conversation
services/webhook/webhook.go
Outdated
| if err != nil { | ||
| log.Error("prepareWebhooks.sigWrite: %v", err) | ||
| } | ||
| signaturegithub = "sha1=" + hex.EncodeToString(sig.Sum(nil)) |
There was a problem hiding this comment.
Thanks for the PR!
It looks like the only difference between the current signatures, and this one, is the sha1= prefix. So rather than storing that in the DB it, it could be added on the fly when headers are set.
There was a problem hiding this comment.
@techknowlogick thanks for the quick reply. This is my first PR and I might be missing some obvious things for sure :)
current signature is in sha256 format and X-Hub-Signature expects sha1, at least that seems to be the standard for GitHub. I tried using sha256= in the header but that doesn't seem to be supported at least with the upstream tool I used. It could be a bug on the other tool which may be hardcoded to read sha1 always.
I also tried using X-Hub-Signature-256 so that I could directly pass the existing signature but also didn't work for me.
Current signature is created with:
sig := hmac.New(sha256.New, []byte(w.Secret))
I create the additional signature with:
sig := hmac.New(sha1.New, []byte(w.Secret))
GiteaBot
added
the
lgtm/need 2
services/webhook/deliver.go
Outdated
| @@ -97,6 +97,7 @@ func Deliver(t *models.HookTask) error { | |||
| req.Header.Add("X-Gogs-Delivery", t.UUID) | |||
| req.Header.Add("X-Gogs-Event", t.EventType.Event()) | |||
| req.Header.Add("X-Gogs-Signature", t.Signature) | |||
| req.Header.Add("X-Hub-Signature", t.SignatureGithub) | |||
There was a problem hiding this comment.
Why doesn't this work?
| req.Header.Add("X-Hub-Signature", t.SignatureGithub) | |
| req.Header.Add("X-Hub-Signature-256", "sha256="+t.Signature) |
There was a problem hiding this comment.
I tested that on my side it doesn't work for me using Ansible AWX as the upstream integration point (https://github.com/ansible/awx/blob/devel/awx/api/views/webhooks.py). It expects X-Hub-Signature only with sha1. It doesn't accept X-Hub-Signature-256.
logger.debug("Expected signature missing from header key HTTP_X_HUB_SIGNATURE")
Not sure if 'X-Hub-Signature-256' is universally supported out there. If there is a reason not to support X-Hub-Signature with sha1 in gitea, I can change the PR to do X-Hub-Signature-256 only and open an issue with Ansible AWX project.
There was a problem hiding this comment.
Ok. If you're certain that the header I suggest above isn't supported I'd suggest we still add it but... in addition we should adjust your PR:
- SignatureGitHub should become SignatureSHA1
- Don't store it with the
sha1=prefix. - Only add the prefix when setting the header.
If we're pre calculating this we're going to need to write a migration to update any stored webhook to add the sha1 signature to the db. Otherwise we'll have to have some logic that tests if the Sha1 signature is empty before emitting the header.
There was a problem hiding this comment.
@zeripath
it makes perfect sense. I put in those changes. In terms of migration, I took a look at it. I might be able to add a migration, I just don't know how to test it properly. Does the migration run on a certain condition or every time?
don't store 'sha=1' in the DB, add when emitting header
Codecov Report
@@ Coverage Diff @@
## main #16115 +/- ##
==========================================
+ Coverage 44.17% 44.20% +0.02%
==========================================
Files 684 694 +10
Lines 82289 82335 +46
==========================================
+ Hits 36354 36393 +39
- Misses 40046 40056 +10
+ Partials 5889 5886 -3
Continue to review full report at Codecov.
|
|
Why do we save the hash in the database? A webhook is usally send once and then the hash is not of use anymore? |
My guess is that because this is the hash of the secret for the integration point which will be used over and over again over the lifecycle of the webhook. Rather than recalculating during message delivery, it's pre-calculated and stored in the DB. |
|
They don't follow the spec btw
|
I think this applies to the receiving end to validate the secret key by recalculating the hash and compare with what's in the header every time. For the originator side (gitea in this case), hash would stay the same given a pre-shared secret key stored in the database. Everytime there is an event that's matched in the webhook, the message is emitted from gitea with the pre-calculated hash from the DB in the header. |
The quote is about validation. It tells you to look in the The hash can't be precalculated because it hashs the content of the send message. |
|
Good points. I see the issue with the current implementation. I think we can close this in favor of #16176 |
This PR removes multiple unneeded fields from the `HookTask` struct and adds the two headers `X-Hub-Signature` and `X-Hub-Signature-256`. ##⚠️ BREAKING⚠️ * The `Secret` field is no longer passed as part of the payload. * "Breaking" change (or fix?): The webhook history shows the real called url and not the url registered in the webhook (`deliver.go`@129). Close #16115 Fixes #7788 Fixes #11755 Co-authored-by: zeripath <[email protected]>
This PR removes multiple unneeded fields from the `HookTask` struct and adds the two headers `X-Hub-Signature` and `X-Hub-Signature-256`. ##⚠️ BREAKING⚠️ * The `Secret` field is no longer passed as part of the payload. * "Breaking" change (or fix?): The webhook history shows the real called url and not the url registered in the webhook (`deliver.go`@129). Close go-gitea#16115 Fixes go-gitea#7788 Fixes go-gitea#11755 Co-authored-by: zeripath <[email protected]>
This PR adds support for Github style webhook integration by adding X-Hub-Signature header to webhook deliveries.
It fixes #7788