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.

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

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

Open
manavo opened this issue Apr 17, 2023 · 16 comments
Open

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

manavo opened this issue Apr 17, 2023 · 16 comments
Assignees
Labels
pending-review request work-scheduled This label indicates that the fix/resolution is scheduled into Vonage's workload

Comments

@manavo
Copy link

manavo commented Apr 17, 2023

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

  • Version used: 4.0.0, 4.1.1
  • Environment name and version (e.g. PHP 7.2 on nginx 1.19.1): Confirmed with PHP 8.1 and PHP 8.3, with Caddy v2.6.4
  • Operating System and version: Ubuntu 20.04
@SecondeJK SecondeJK self-assigned this May 2, 2023
@SecondeJK SecondeJK added the work-scheduled This label indicates that the fix/resolution is scheduled into Vonage's workload label May 2, 2023
@SecondeJK
Copy link
Contributor

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
Copy link
Author

manavo commented Aug 5, 2023

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
Copy link
Contributor

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

@SecondeJK SecondeJK reopened this Aug 7, 2023
@SecondeJK SecondeJK added pending-review and removed work-scheduled This label indicates that the fix/resolution is scheduled into Vonage's workload labels Aug 7, 2023
@manavo
Copy link
Author

manavo commented Aug 7, 2023

Thanks @SecondeJK!

@SecondeJK
Copy link
Contributor

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
Copy link
Author

manavo commented Aug 9, 2023

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
Copy link
Contributor

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

@manavo
Copy link
Author

manavo commented Aug 16, 2023

Thanks @SecondeJK! Really happy to hear that!

@manavo
Copy link
Author

manavo commented Feb 26, 2024

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

@manavo
Copy link
Author

manavo commented Nov 18, 2024

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!

@pardel pardel reopened this Nov 18, 2024
@SecondeJK
Copy link
Contributor

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

@SecondeJK SecondeJK added request work-scheduled This label indicates that the fix/resolution is scheduled into Vonage's workload labels Nov 19, 2024
@manavo
Copy link
Author

manavo commented Nov 19, 2024

Thanks @SecondeJK!

@SecondeJK
Copy link
Contributor

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
Copy link
Author

manavo commented Nov 21, 2024

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
Copy link
Contributor

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)

  • Hash the body of the request as a complete JSON string, signed with the Signature Secret.
  • To verify, check the hash against the payload_hash in the JWT. The JWT itself can be checked with verifySignature

@manavo
Copy link
Author

manavo commented Dec 3, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-review request work-scheduled This label indicates that the fix/resolution is scheduled into Vonage's workload
Projects
None yet
Development

No branches or pull requests

4 participants