GitHub Webhook Validation issues

[pseudomorph]

There seems to be some issue with the hash validation on GH release events.

SHA-256 seems to come in over the 'X-Hub-Signature-256' header, and is calculated by a combination of the request body and the secret key, as per: https://docs.github.com/en/webhooks/webhook-events-and-payloads . Right now Terrareg appears to be passing an empty byte string to the HMAC function and is attempting to pull the SHA-256 hash from the 'X-Hub-Signature' header, which is a SHA-1 hash. I've tried a few local changes, but am struggling to get the auth to work. I'll post if I find any solution here.

[pseudomorph]

My bad, the request body is being accounted for.

The following appears to fix the issue in terrareg/server/api/module_version_create_github_hook.py:

              # Validate signature
             if terrareg.config.Config().UPLOAD_API_KEYS:
                 # Get signature from request
+                request_signature = request.headers.get('X-Hub-Signature-256', '')
-                request_signature = request.headers.get('X-Hub-Signature', '')
                 # Remove 'sha256=' from beginning of header
                 request_signature = re.sub(r'^sha256=', '', request_signature)
                 # Iterate through each of the keys and test

[pseudomorph]

On a separate note: it looks like a previous version of a module must be published before GH events result in automatic publishing. Would it be possible to allow direct publishing of module versions using GH webhooks without requiring a previous version to have been published?

[MatthewJohn]

Hey @pseudomorph ,

Thanks for getting in contact - sorry about the issues with the headers - I'll take a read too and see if I can find anything!

I'm afraid I didn't have much of a hand in writing the Github hook (maybe @Davidsoff may know more :) )

In terms of the GH webhooks publishing vs indexing and then publishing - as far as I was are, the Github hooks should do both (as opposed to Bitbucket that just does the indexing and not the publishing) - I'll take a look into this and let you know

Many thanks Matt

[MatthewJohn]

Sorry @pseudomorph.. Yes, I see the issue with publishing the modules - I assume you have AUTO_PUBLISH_MODULE_VERSIONS set to true, calling the github hook and then it indexes the modules but doesn't appear to publish it correctly.

The cause of this is due to that the indexer "lazily" sets the published state of the module in the database.. but this isn't all that's required and it should call calling the publish method on the module, which updates some cache value of the module with the latest version. I'm sorry about this - I'll create a separate ticket for this and address it, if that's okay?

[MatthewJohn]

Created https://gitlab.dockstudios.co.uk/pub/terrareg/-/issues/425 for this second issue

[MatthewJohn]

Interestingly, according to the link you provided:

X-Hub-Signature: This header is sent if the webhook is configured with a [secret](https://docs.github.com/en/rest/repos#create-hook-config-params). This is the HMAC hex digest of the request body, and is generated using the SHA-1 hash function and the secret as the HMAC key. X-Hub-Signature is provided for compatibility with existing integrations, and we recommend that you use the more secure X-Hub-Signature-256 instead.

So it appears that they should be passing the X-Hub-Signature attribute, even if it is for legacy purposes (and we should migrate).

I'm afraid I don't currently have a publicly accessible instance of Terrareg available to test against github. If there a way you could capture a payload from github to provide at all - I know this is a big ask (if not, I can mock some fake public API up capture a payload). If you do manage to get one, if you want to keep it "secure" feel free to email me (email address in my Github bio) (feel free to redact anything sensitive, of course :) )

Many thanks in advanced Matt

[pseudomorph]

No worries, and thanks for the fast response!

The format of the headers WRT signature is as follows:

"X-Hub-Signature: sha-1=...." "X-Hub-Signature: sha-256=..."

This lines up with what the docs describe and what I observe in the wild. Since Github provides both, it's probably best to go with the latter option for greater security.

Thanks again!

[pseudomorph]

And I can provide a mock output shortly.

[MatthewJohn]

Sorry @pseudomorph, I mistook your reply.

Yes, you're absolutely right, that change does fix the issue.

This lines up with what the docs describe and what I observe in the wild. Since Github provides both, it's probably best to go with the latter option for greater security.

Absolutely and they suggest the non-sha256 is legacy anyway, so no point changing SHA256 functionality to SHA1 to support this :)

I spun up a public API to get a real call and created a test to use this. Have got a fix in: https://gitlab.dockstudios.co.uk/pub/terrareg/-/merge_requests/348

Assuming that you're happy with it, I'll get it as soon as pipeline passes :)

Many thanks Matt

[pseudomorph]

Looks good to me!

I really appreciate the help on this.

[MatthewJohn]

No problem :) Apologies for the bug :)

[pseudomorph]

No sweat, this is a fantastic project!

I've deployed the latest release and can confirm it works for me.

[MatthewJohn]

Thank you :) That's great to hear

With regards to the other issue, if it is the glaring issue that I saw when I first looked at it, this PR should fix it: https://gitlab.dockstudios.co.uk/pub/terrareg/-/merge_requests/349

[MatthewJohn]

Hey @pseudomorph, I've merged the fix for auto publishing, which is released in v2.75.2 :)

Please let me know if there's any other issues :)

Many thanks Matt