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.

Sign up for GitHub

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

Jump to bottom

Add X-Hub-Signature header to webhook deliveries #16115

Closed
wants to merge 5 commits into from
Closed

Add X-Hub-Signature header to webhook deliveries #16115

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d66fd38
Add X-Hub-Signature header to webhook deliveries
oatakan Jun 8, 2021
234318d
use tab to fix lint check
oatakan Jun 9, 2021
5656a49
try to fix windows lint by using tabs
oatakan Jun 9, 2021
74159c2
use gofmt -w
oatakan Jun 9, 2021
749cafa
change variable name to SignatureSHA1
oatakan Jun 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions models/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ type Webhook struct {
IsSystemWebhook bool
URL string `xorm:"url TEXT"`
Signature string `xorm:"TEXT"`
SignatureGitub string `xorm:"TEXT"`
HTTPMethod string `xorm:"http_method"`
ContentType HookContentType
Secret string `xorm:"TEXT"`
Expand Down Expand Up @@ -654,6 +655,7 @@ type HookTask struct {
Typ HookTaskType `xorm:"VARCHAR(16) index"`
URL string `xorm:"TEXT"`
Signature string `xorm:"TEXT"`
SignatureGithub string `xorm:"TEXT"`
api.Payloader `xorm:"-"`
PayloadContent string `xorm:"TEXT"`
HTTPMethod string `xorm:"http_method"`
Expand Down
1 change: 1 addition & 0 deletions services/webhook/deliver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't this work?

Suggested change
req.Header.Add("X-Hub-Signature", t.SignatureGithub)
req.Header.Add("X-Hub-Signature-256", "sha256="+t.Signature)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zeripath

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@oatakan oatakan Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

req.Header["X-GitHub-Delivery"] = []string{t.UUID}
req.Header["X-GitHub-Event"] = []string{t.EventType.Event()}

Expand Down
36 changes: 26 additions & 10 deletions services/webhook/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package webhook

import (
"crypto/hmac"
"crypto/sha1"
"crypto/sha256"
"encoding/hex"
"fmt"
Expand Down Expand Up @@ -179,17 +180,32 @@ func prepareWebhook(w *models.Webhook, repo *models.Repository, event models.Hoo
signature = hex.EncodeToString(sig.Sum(nil))
}

var signaturegithub string
if len(w.Secret) > 0 {
data, err := payloader.JSONPayload()
if err != nil {
log.Error("prepareWebhooks.JSONPayload: %v", err)
}
sig := hmac.New(sha1.New, []byte(w.Secret))
_, err = sig.Write(data)
if err != nil {
log.Error("prepareWebhooks.sigWrite: %v", err)
}
signaturegithub = "sha1=" + hex.EncodeToString(sig.Sum(nil))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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))

}

if err = models.CreateHookTask(&models.HookTask{
RepoID: repo.ID,
HookID: w.ID,
Typ: w.Type,
URL: w.URL,
Signature: signature,
Payloader: payloader,
HTTPMethod: w.HTTPMethod,
ContentType: w.ContentType,
EventType: event,
IsSSL: w.IsSSL,
RepoID: repo.ID,
HookID: w.ID,
Typ: w.Type,
URL: w.URL,
Signature: signature,
SignatureGithub: signaturegithub,
Payloader: payloader,
HTTPMethod: w.HTTPMethod,
ContentType: w.ContentType,
EventType: event,
IsSSL: w.IsSSL,
}); err != nil {
return fmt.Errorf("CreateHookTask: %v", err)
}
Expand Down