Adds v3 webhook signature verification. #326
Conversation
950f552 to
60bf467
Compare
60bf467 to
d271d14
Compare
| // | ||
| // See https://developer.pagerduty.com/docs/webhooks/webhook-signatures/ for more details. | ||
| // | ||
| func VerifySignature(payload []byte, header, secret string) error { |
There was a problem hiding this comment.
My concern with this approach is that implementation details of the PagerDuty API leak outside of the package into the consumer's program, specifically needing to know which header to look for and pull it out.
How would you feel about taking the entire *http.Request as the first argument, and then the secret as the second? That way the headers and what-not are not exposed to the consumer. After reading the body we could replace it with a ioutil.NopCloser(bytes.Reader(body)) so that consumers can then read the request body themselves.
There was a problem hiding this comment.
Thinking more, if we do this we may need another sentinel error for an invalid request (missing / malformed header).
There was a problem hiding this comment.
Yeah that's not a bad idea.
The intention was definitely to build more helpful public API methods on top of this one. Was just trying to get some initial code out in public since we have folks asking about how this would be implemented.
There was a problem hiding this comment.
@theckman : actually I am kind of thinking that the method you are describing might be in addition to this one. Something like:
// VerifyWebhookRequest extracts and verifies the webhook signature.
// This method would likely return more detailed errors that differentiate between missing / invalid signatures, etc.
func VerifyWebhookRequest(request *http.Request, secret string) erroror
// ConstructWebhookEvent verifies and parses a v3 Webhook Event
func ConstructWebhookEvent(request *http.Request, secret string) WebhookEvent, errorIt still seems useful leave this more raw public method for clients wishing to have more control over this functionality?
There was a problem hiding this comment.
My personal preference generally is to try and keep the API as small as we can, as it means there's less for us to maintain and honestly less to potentially break by accident in a future update. Text communication is hard, so maybe it'd be best if I try to figure out what assumptions / expectations I think I have:
- I believe the majority of Gophers are working within an environment where the request will be available to them as an
*http.Request. - Once it's out of early access, I don't expect the V3 signature format to change much (especially the header) without it incurring a new version designation from PagerDuty.
- Considering it's unlikely to change, someone could lift the verification code out of this project and put it into theirs (or be inspired by it to write their own) if our API signature doesn't work for them.
If we get issues + comments indicating that the *http.Request format doesn't work for a good many, we can look to expose that additional functionality in a future 1.x minor release. If we make the API too wide right away, we must release a major version if we realize nobody is using it and no longer wish to support it.
Edit: Since I've become more attentive to this project, my goal is to not have 6 months between minor releases going forward. I think as we get new features, we should roll them out as minor versions and do it as soon as they are merged. That way if we do get this request, and we do choose to add it, someone isn't waiting awhile to take advantage of it.
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
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
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
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
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
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
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
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
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
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
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
Adds basic functions to verify v3 webhook signatures into the client library.