webhook: restrict incoming requests

[HumorBaby]

A bad actor can use this module to spam a channel with custom content. All that is needed is currently the bot's IP/hostname (can be gotten from .help), a port for the web server (test the default in the source, then port scan), the name of the channel that is currently displaying GitHub webhooks (easy to guess/hard to hide), and a specially formatted JSON payload (format is defined in the source).

GitHub has built-in measures to ensure that payloads going to an endpoint can be verified as coming from GitHub itself. A secret token that is known to the endpoint server and the GitHub webhook is used to hash/sign the payload, which can then be verified server-side.

This PR implements this functionality.

Relevant settings:

[github]
webhook_secret  = ...  # If this setting is not `None` (default when not set), then a `POST /webhook` will enforce verification
debug_mode = ...  # When `True`, this setting allows requests to pass through verification, as well as enables the `GET /webhook` endpoint for testing

[dgw]

OK, this is what I have so far. This is a good feature!

Per IRC discussion I know you're already planning to move the functions nested inside verify_request out, so no need for me to mark them with line notes.

[HumorBaby]

Initially I had though to always repond with 400 Bad Request so a malicious client couldn't get information about what was actually wrong with the request, but I think that's overkill 😅 Since one of the requested changes was a status code for GET /webhook, I decided to update the rest, so now failed webhooks actually originating from GitHub will be more informative (since responses are stored).

Also, you got me thinking about confirming that Bottle.py would handle the requests/aborts properly.... so, I spammed my test server with 1000 in parallel (split over 32 processes) requests of various valid/invalid proportions. Here is one of the result sets:

Valid requests    --> 468 sent
  468 correct response(s)
    0 incorrect response(s)

Invalid requests  --> 532 sent
  532 correct response(s)
    0 incorrect response(s)

The test and results were generated from this: https://gist.github.com/HumorBaby/32d4414c1956554d0f9237b41960bc8a

[HumorBaby]

✌️ more informative comments

[dgw]

Let's call it ready. Maybe this will go into 0.2.0 after all! haha

Could use some squashing though. If you feel like it. Meh.

[HumorBaby]

Could use some squashing though

Goodbye @dgw co-authorships 😜

[dgw]

Goodbye @dgw co-authorships

I believe in your ability to use git rebase -i and preserve them if you think it's worth doing. 😛

[HumorBaby]

Final tests (with checking of actual response codes; test gist updated):

Valid requests    --> 174 sent            
  174 correct response(s)                 
    0 incorrect response(s)               
                                          
Invalid requests:                         
    Missing Signatures     --> 192 sent   
      192 correct response(s)             
        0 incorrect response(s)           
    Mismatched Signatures  --> 191 sent   
      191 correct response(s)             
        0 incorrect response(s)           
    Unsupported Digest     --> 218 sent   
      218 correct response(s)             
        0 incorrect response(s)           
    GET /webhook           --> 208 sent   
      208 correct response(s)             
        0 incorrect response(s)           

Note: 17 requests were dropped. I'd guess bottle became overloaded during testing, given error: [Errno 32] Broken pipe server-side.

[dgw]

Cursory search indicates that yes, it's probably because Bottle took too long and the test script closed the connection before Bottle actually started sending a response. We're probably using the single-threaded wsgiref server, which can only process one request at a time.

Such heavy loads aren't likely in a real world deployment unless a lot of busy repos are connected to a single bot. But regardless, better webserver scaling is a separate issue (possibly addressed by #22 and/or switching Bottle to another server adapter).

[HumorBaby]

🤭 Forgot to change one of the bottle.aborts to abort_request...

[dgw]

Oof, those merge conflicts.

[HumorBaby]

Oof, those merge conflicts.

Was actually pretty straightforward hehe. I bet it had something to do with the 100+ commits to make master prettier since this PR was originally approved 🐱

[dgw]

So I still need to test this, but meanwhile: 18 months later, some Sopel API stuff changed a bit in the run-up to 7.0, and is worth tweaking.

But more importantly, GitHub documents an updated signature method (SHA-256 instead of SHA-1) and header name (X-Hub-Signature-256 instead of X-Hub-Signature). The header this patch uses is included "For backward-compatibility".

What's funny is I was thinking about just shipping this in 0.4.0 anyway, but looking at it again brought up this stuff I'd rather have changed first.

[dgw]

As far as I can tell, this is never used. The event is added to the payload as payload['event'] below (R206).

Suggested change

event = bottle.request.headers.get('X-GitHub-Event') or 'ping'

[dgw]

I'm honestly shocked that this doesn't have any merge conflicts yet.

Accidentally deleted an old suggestion from 2020, but it's OK. I put it back along with the other stuff I chose to suggest today.

[dgw]

@HumorBaby if by chance you haven't turned into RipVanBaby due to lack of attention… I updated the test script to work on py3, and test against the SHA256 behavior. See revisions: https://gist.github.com/dgw/21674014960259586569d85bae2d4830/revisions

Edit: Stable af.

Valid requests    --> 199048 sent
199048 correct response(s)
    0 incorrect response(s)

Invalid requests:
    Missing Signatures     --> 200891 sent
    200891 correct response(s)
        0 incorrect response(s)
    Mismatched Signatures  --> 200437 sent
    200437 correct response(s)
        0 incorrect response(s)
    Unsupported Digest     --> 199617 sent
    199617 correct response(s)
        0 incorrect response(s)
    GET /webhook           --> 200007 sent
    200007 correct response(s)
        0 incorrect response(s)