-
Notifications
You must be signed in to change notification settings - Fork 240
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
Add v3 webhook signature verification #370
Conversation
I do want to think on this a little bit. It may make sense to put the Webhook related code within its own package, |
93fd261
to
a1fc6be
Compare
I've done just that and reserved a merge conflict in the process (since I added a blurb to the README). @stmcallister this is ready for your 👀 too. |
a12087d
to
a0154c8
Compare
Will have a look at this later today. Thanks for picking this back up @theckman. |
a0154c8
to
1282d60
Compare
This change is an extension of the PR raised by @ChezCrawford (#326), where the requested changes have been applied to the branch and all commits have been squashed. This adds a new function to the package, VerifySignatureWebhookV3, which accepts an *http.Request and a secret, and validates that the request is properly signed using that secret. This function does return some sentinel error values so that you can choose which HTTP status to send back to the caller. Supersedes #326
1282d60
to
3df5eb4
Compare
Most importantly, I really appreciate you reviving this @theckman. I personally would like to get some working open-source code out there that cleanly implements signature verification. (Developers often have trouble getting this exactly right just by reading documentation like this.) I like moving this stuff into a separate package. Do you have a use-case for using this function signature in an application: When we talk about avoiding "breaking changes" to the library's public API in future releases, this method still feels like a slightly strange boundary to me. While it accepts the I still personally feel like the most useful set of public APIs would be something like: func VerifySignature(payload []byte, header, secret string) error
func ExtractWebhookEvent(r *http.Request, secret string) (WebhookEvent, error) That second method would give kind of the "all in one" functionality while exposing the first method doesn't seem like a huge detriment (especially if it lives in a webhooks-specific package 😉 ) since it would also be used by the second method... Edit: if you like that idea, I am happy to make the necessary modifications. |
@ChezCrawford In general I try to qualify the validity of requests before I hand them to my actual handlers. So I'd probably verify the signature before trying to parse it into actual data structures. If memory were a constraint, I may combine them for efficiency purposes. This may be relevant for #367, but based on the shape of the JSON documents for V3 Webhooks, I'm not confident we will be able to implement a single function to deserialize the response and provide a single useful data structure for the consumer to use. V3 Webhooks do that thing where you need to have a two-phase deserialization, because the That said, I do think it would make sense to offer an API to at least extract the top-level document to allow them the ability to do that second-phase deserialization, and maybe a helper one that combines them.
Edit: Forgot to add this. A big goal of mine is still to avoid consumers needing to know anything about the |
Ah I probably should have clarified. When I proposed the two methods above, I was thinking that the second The My thought was that second method would be the "all in one" approach (take the http request, verify the signature, return the partially deserialized event) and that the To me, it seems like this current method is a strange mix of not quite utility yet not quite "all in one" either. However, this may be something that is somewhat common in Go and just a little new to me. If you're happy the way it is, i'll drop my approval. |
Actually one final question here: in your mind, what do the full method signatures look like for the following three methods? If you can help me get on the same page around a final set of method signatures, then we can get this into 1.5 and I might attempt the code to add in the actual events for #367. |
In Go it's not uncommon to see composable APIs. So while you may have a function that does all the actions in one shot if you want, you may want to do each step separately. For example, maybe you put the Webhook events on a message bus and don't actually want to deserialize at that layer. For function signatures, I was thinking maybe something eventually like: func VerifySignature(r *http.Request, secret string) error
func ExtractPayload(r *http.Request, secret string) (Payload, error)
func VerifyAndExtractPayload(r *http.Request, secret string) (Payload, error) |
|
||
orb := r.Body | ||
|
||
b, err := ioutil.ReadAll(io.LimitReader(r.Body, webhookBodyReaderLimit)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: A colleague of ours (@dobs ) mentioned that MaxBytesReader might be a good fit here. Does that make sense to you?
Alright fair enough. Thanks for the clarification. I left one small comment but i'll go ahead and approve since we can work with this. |
This change is an extension of the PR raised by @ChezCrawford (#326), where the
requested changes have been applied to the branch and all commits have been
squashed.
This adds a new function to the package, VerifySignatureWebhookV3, which accepts
an *http.Request and a secret, and validates that the request is properly signed
using that secret. This function does return some sentinel error values so that
you can choose which HTTP status to send back to the caller.
Supersedes #326