Vonage / vonage-php-sdk-core

Vonage REST API client for PHP. API support for SMS, Voice, Text-to-Speech, Numbers, Verify (2FA) and more.
https://developer.vonage.com/
Apache License 2.0
922 stars 181 forks source link

`new \Vonage\Client\Signature` fails for `input` `eventUrl` callback #401

Open manavo opened 1 year ago

manavo commented 1 year ago

When getting a callback to capture an input (dtmf or speech, as defined in the NCCO), the resulting data that gets POSTed to the eventUrl endpoint.

In that data though, there are nested objects and it's all POSTed as JSON.

Example data (from here):

{
  "speech": { "results": [ ] },
  "dtmf": {
    "digits": "1234",
    "timed_out": true
  },
  "from": "15551234567",
  "to": "15557654321",
  "uuid": "aaaaaaaa-bbbb-cccc-dddd-0123456789ab",
  "conversation_uuid": "bbbbbbbb-cccc-dddd-eeee-0123456789ab",
  "timestamp": "2020-01-01T14:00:00.000Z"
}

So if we call this:

$signature = new \Vonage\Client\Signature($allRequestData, $signingSecretKey, 'sha512');

We end up getting an error of converting an Array to string here:

https://github.com/Vonage/vonage-php-sdk-core/blob/main/src/Client/Signature.php#L58

I'm not sure how it should get encoded

Expected Behavior

No error should be thrown

Current Behavior

An error is thrown, and we cannot validate the payload_hash from the JWT Authorization header

Possible Solution

Not sure how the encoding happens on Vonage's side when creating the webhook, but we need to re-create that here so we can validate these webhooks too. I just don't know how the encoding is happening on the other side, in order to have the same logic here.

Steps to Reproduce (for bugs)

  1. $data = json_decode('{ "speech": { "results": [ ] }, "dtmf": { "digits": "1234", "timed_out": true }, "from": "15551234567", "to": "15557654321", "uuid": "aaaaaaaa-bbbb-cccc-dddd-0123456789ab", "conversation_uuid": "bbbbbbbb-cccc-dddd-eeee-0123456789ab", "timestamp": "2020-01-01T14:00:00.000Z" }', true);
  2. new \Vonage\Client\Signature($data, 'secret', 'sha512');

Context

Your Environment

SecondeJK commented 1 year ago

Signature takes an array, so cast the incoming object to an array (I assume right now it's a stdObject?)

$payload = json_decode($data, true, 512, JSON_THROW_ON_ERROR);
$signature = new \Vonage\Client\Signature($payload, 'secret', 'sha512');
$signature->check();
manavo commented 1 year ago

Hi @SecondeJK, in my example it already is an array, it's the nested data which create the issue.

If I'm not mistaken, this is the line that creates the issue:

CleanShot 2023-08-05 at 19 46 06@2x

Since $value could be an array in the case of nested data. And just casting the array to a string will cause the issue.

Does that help explain it a bit more and why this should be re-opened?

SecondeJK commented 1 year ago

The nested data makes sense - I'm reopening and have asked the question internally with the engineering team to see what they come back with. It -might- be the case that checking these types of webhook isn't recommended or supported

manavo commented 1 year ago

Thanks @SecondeJK!

SecondeJK commented 1 year ago

Hi @manavo Unfortunately, this isn't going to be the response you want, but after talking it through with engineering it seems there are some nuances about this particular webhook that mean the best advice I can give is recommending not to check the signature on it.

Checking the signature is -mainly- designed for SMS/Messaging, but you'll probably notice that when setting up Voice hooks it will sign them by default. This works fine for any incoming webhook that has a flat structure, but as soon as you have a more complex one (such as the nested object in the dtmf capture) it will fail out of the box as you've seen.

The way the backend puts together the string for validation is using that code you've pasted in above where the string replace is done - the expected string is a url query string. So, in theory, you could hack a string together to flatten it but I wouldn't recommend it. Apologies that there's not really a solution to this.

manavo commented 1 year ago

Thanks @SecondeJK, definitely not what I was hoping to hear, but understand that it might be a bit trickier to do. Hopefully it will be implemented at some point, so these endpoints can also be secured! (otherwise even adding the signature is kind of pointless?)

Either way, thanks for digging into it!

SecondeJK commented 1 year ago

Thanks for understanding. A conversation is now underway to look at how to change how the signature is checked to fix this

manavo commented 1 year ago

Thanks @SecondeJK! Really happy to hear that!

manavo commented 9 months ago

Hey @SecondeJK, just wanted to check if there was any progress with the signature checking?

manavo commented 2 weeks ago

Hi @SecondeJK, just wanted to check in once more and see if there was any update with this? Can we at least keep this issue open, since it is an actual issue?

Thanks!

SecondeJK commented 2 weeks ago

Looking back into this now, I'll see how far the conversations went on the behaviour

manavo commented 2 weeks ago

Thanks @SecondeJK!

SecondeJK commented 1 week ago

OK so the approach to verifying the signature is wrong in this implementation - checking an incoming is signed works for SMS webhooks but elsewhere you need to check via. the JWT library.

To verify the incoming payload, use the following method:

Vonage\JWT\TokenGenerator::verifySignature()

So the procedure is slightly different, where you extract the token and add your secret.

manavo commented 1 week ago

Hi @SecondeJK,

Thanks for the update on this!

If I'm not mistaken though, that would be the validating that the JWT signature is correct (which is also important to check).

Don't we then need to check the payload_hash claim in the JWT, which would the the signature of the body/payload? (https://developer.vonage.com/en/getting-started/concepts/webhooks)

That's the part that's failing here, generating the signature using \Vonage\Client\Signature. I also just tried generating the hash using hash_hmac('sha256', $rawRequestBody, $webhookSigningKey));, but the signatures didn't match. Which might be because of the sorting or params etc that the \Vonage\Client\Signature class does.

Thanks!

SecondeJK commented 3 hours ago

OK, so here is the correct way to sign for voice callbacks (I'll be adding documentation into the readme for this, and to our docs site)

manavo commented 3 hours ago

Thanks @SecondeJK, can you add another comment here when the docs are ready? This sounds like the last thing I tried (hashing the raw request body, which I think is JSON anyway, right?) but couldn't get it to work. So might need the docs there to get it right!