[FUNK-2000] Fixed extensible webhook shared secret issue #3316
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3316 +/- ##
=======================================
Coverage 80.03% 80.03%
=======================================
Files 1202 1202
Lines 22105 22112 +7
Branches 4355 4359 +4
=======================================
+ Hits 17691 17697 +6
Misses 3637 3637
- Partials 777 778 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joe-ayoub-segment
added
not-partner-eng-review (cloud)
partner-eng-review (cloud)
and removed
not-partner-eng-review (cloud)
labels
|
Hi @vaibhav-nanda ping me here when you need this reviewed. |
| if (payload) { | ||
| const payloadData = payload.length ? payload[0]['data'] : payload['data'] | ||
| if (settings.sharedSecret && payloadData) { | ||
| const digest = createHmac('sha1', settings.sharedSecret) | ||
| .update(JSON.stringify(payloadData), 'utf8') | ||
| .digest('hex') | ||
| xSignatureHeader = { 'X-Signature': digest } | ||
| } | ||
| } |
There was a problem hiding this comment.
Hi @vaibhav-nanda I have a few questions about this:
-
Do we need the xSignatureHeader to always be present if the customer provides a sharedSecret?
-
How do you know that you are generating the xSignatureHeader correctly?
-
This Action implements the performBatch() function. If we send a batch of events then I assume the xSignatureHeader needs to created based off of all the data being sent - is that correct?
-
Shouldn't the xSignatureHeader value be generated from the actual payload being sent to the destination platform, rather than the input payload values? I can see that the data object gets encoded before it gets sent.
if (data) return encodeBody(data, contentType)
There was a problem hiding this comment.
-
Yes, the x-signature header is expected to be present always, if the customer has provided a sharedSecret.
-
This was confirmed by verifying the header being sent on the destination. The header received on destination was matched with the value calculated using the code on Line 37, and it was the same. This was also confirmed by sending the same request on Actions Webhook destination (it has the same functionality for x-signature header) and it also had the same header as the extensible webhooks.
-
As per the current implementation in Actions Webhook destination, only the first event in the payload is being used to generate the xSignatureHeader. The same is being followed over here as well.
-
It has been confirmed by comparing the xSignatureHeader values from Actions webhook and Extensible Webhook destinations. Both are sending out the same headers for the same request to both of them. The xSignatureHeader value is same for both the scenarios.
There was a problem hiding this comment.
Thanks @vaibhav-nanda,
Regarding 1. Makes sense.
Regarding 2. Makes sense.
Regarding 3 and 4: This doesn't make sense to me. Shouldn't the entire body be used? How will the receiving side know how to check which part of the body was used when it's checking if the request is valid? Because of this I think we should ask @varadarajan-tw to take a look. I'm not qualified ;)!
There was a problem hiding this comment.
It seems like for webhook action destination, we call out the signature computation logic in the docs.
https://segment.com/docs/connections/destinations/catalog/actions-webhook/#shared-secret-with-batching. The customer should be able to validate by parsing the body, computing signature and validating it against the the header sent by Segment.
However, the general practice is to compute it for the entire request body. I don't see a standard at Segment but Twilio's signature signing standard seems to be a good one to follow. We can implement this and call this out in our docs so that customers know how to validate.
My only concern with batch request signing is performance when dealing with large payloads. If it doesn't add too much overhead for a batch of 1000 requests, we should consider signing the entire payload.
There was a problem hiding this comment.
Thanks @varadarajan-tw - makes sense.
https://twilio-engineering.atlassian.net/browse/FUNK-2000
The X-Signature is not being sent for extensible webhook right now, although the UI provides an option to add the sharedSecret just like webhooks destination.
Testing
This is tested in staging. The shared secret val is being used to send the required data in
x-signatureheader.