Utility to validate webhook signatures

[stof]

I'm not sure this should actually be implemented in this repo, as it is different from other use cases, but maintainers might still decide it fits this repository.

When implementing github webhooks, it is possible to secure them (to reject any fake requests sent to the webhook from elsewhere than Github).

Github sends a signature header in each webhook request. Their documentation shows the Ruby code to validate the request: https://developer.github.com/webhooks/securing/

I suggest providing a utility method which would validate a signature, to avoid having each developer figuring out the corresponding PHP code:

class SignatureChecker
{
    function validateSignature(string $signature, string $requestBody, string $secret): bool
    { // TODO implement that
    }
}

The naming is not final of course.

I suggest that this API takes the 3 strings as arguments rather than alternatives. Here are the ones I considered but which I consider inferior:

• the secret could be a constructor argument, but this could make things harder if you manage multiple webhooks in your codebase and each of them uses a separate secret (you would have to manage separate instances)
• the signature and body could be replaced by a PSR-7 request, from which they would be extracted. But that would make things harder to use for people using frameworks not relying on PSR-7 (Symfony for instance). Having to convert the request to PSR-7 just to check the signature is adding unnecessary overhead (getting the request body and a header are easy enough to be left to the calling code)

[stof]

Actually, looking at how this gets implemented, it might be easy enough to be left outside this package (choice is up to maintainers):

$sig = 'sha1='.hash_hmac('sha1', $requestBody, $secret);

return hash_equals($sig, $signature);

[acrobat]

I actually like the idea! I had to implement (copy the code after the first time) the same check a few times. So I think a utility class for this check can easily be added and help developers in the future. The code I created in the past

list($userSignatureAlgo, $userSignatureHash) = explode('=', $userSignature);
$signature = hash_hmac($userSignatureAlgo, $userBody, $secret);

if (!hash_equals($signature, $userSignatureHash)) {
     throw new InvalidSignatureException('Signatures didn\'t match!');
}

So i'm 👍 on including it here in a small helper class. Any thoughts @Nyholm?

[stof]

@acrobat your solution lets the input control the hashing algo, with no validation of the provided value. That's exactly how all JWT implementation out there (in all languages) faced a security issue allowing to bypass the signature check (as they could control the algo used to validate it). Github says they always use sha1. So until they migrate to a different algo, it is much safer to accept only that one (and the day they support another one, the logic choosing the algo should only allow using sha1 and the new one, not any algo known by PHP).

[stof]

and for the throwing, I would not make it part of the validation logic, as it makes it harder in case they want a different behavior for this case.

[m1guelpf]

@stof Then just catch the exception

[acrobat]

@acrobat your solution lets the input control the hashing algo, with no validation of the provided value. That's exactly how all JWT implementation out there (in all languages) faced a security issue allowing to bypass the signature check (as they could control the algo used to validate it). Github says they always use sha1. So until they migrate to a different algo, it is much safer to accept only that one (and the day they support another one, the logic choosing the algo should only allow using sha1 and the new one, not any algo known by PHP).

Hmm 🤔 you are indeed right, thanks for pointing that out!

Can you start a PR for it @stof? So we can discuss further changes there (like throwing an exception or not), thanks!

[stof]

@m1guelpf having an API turning a boolean into an exception that you need to catch again, to either not throw or throw a different exception suited to your needs ? Why not keeping a utility returning the boolean and letting users decide what they want to do with it ? This SDK has no idea how the code surrounding the signature check looks like, as it is not a framework for building github webhooks.

@acrobat the question is whether the 2 LOC of https://github.com/KnpLabs/php-github-api/issues/741#issuecomment-420230082 are worth building a utility for that (or just documenting them in the README for people wondering the equivalent of the Ruby code provided in the github doc).

For what it is worth, none of the official Octokit SDKs for node.js, Ruby, .NET or go are providing it (I haven't check the ObjC one). The only place I found this logic is in https://github.com/octokit/webhooks.js which is a framework for implementing webhook-based services (used at the core of probot). So I'm not even sure this should be part of this SDK.

and if we decide to put it in this package, where should it live ? It does not really fit into the existing architecture, as it is not about accessing an API.

[m1guelpf]

yeah, that's true, it doesn't fit in the current structure... 🤔

[stof]

Note that when I opened the initial issue, I hadn't looked for the implementation of computing the signature yet. I was fearing it would require using the openssl APIs (which are easy to misuse). But the fact that it is just hash_hmac('sha1', $requestBody, $secret) makes it dead easy.

[acrobat]

@stof Maybe just a docs entry with a minimul code example is better then?