webhooks.net icon indicating copy to clipboard operation
webhooks.net copied to clipboard

Support hot-swapping of webhok secrets

Open dbartol opened this issue 1 year ago • 5 comments

Resolves #261 Resolves #24


(In draft until I finish implementing tests)

Behavior

Before the change?

  • In both ASP.NET Core and Azure Functions, the webhook secret for the webhook processor must be specified when the processor is registered, and cannot be changed afterward. In addition, there can be only one secret specified.

After the change?

  • A client now has the option of specifying a callback delegate to retrieve the list of secrets to validate against. This callback will be invoked for each request, so the results can change dynamically to allow rotation of the secret. In addition, the callback can return multiple secrets, with the request's signature required to match at least one of those secrets.

Other information

Since the request validation code that I was modifying was duplicated between the ASP.NET Core and Azure Functions packages, I took the opportunity to refactor the validation code to allow sharing (see #24). I chose to add the new ValidateWebhookRequestAsync() method to the existing WebhookEventProcessor class. It could go in a separate class, but I figured that most clients that process webhooks will also need to validate them. I'm tempted to also add a new ValidateAndProcessWebhookAsync() method that does both the validation and the processing, since they take essentially the same parameters and will almost always be used together. I didn't go that far in this PR, though.

The parameter types for the new ValidateWebhookRequestAsync() method include the request headers (as the same type used for ProcessWebhookAsync()), the body Stream, and the list of secrets to use. Allowing multiple secrets allows secret rotation to happen more smoothly, because the consumer can continue to process webhooks signed with the old secret for a while until everything starts using the new secret.

Passing the body as a Stream seems to be the least common denominator between the ASP.NET Core and Azure Functions packages, and is likely useful outside of those two scenarios. I considered just passing the body as a string, but that would make it more difficult to implement a defensive check against a maliciously large payload. That check was suggested in #24, but I have not implemented it here.

Validation errors are reported by throwing a WebhookValidationException. This class contains a WebHookValidationError enum that allows clients to determine exactly what went wrong. The two existing clients use this to determine what log message to emit, and what response text, if any, to return.


Additional info

Pull request checklist

  • [ ] Tests for the changes have been added (for bug fixes / features)
  • [ ] Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • [ ] Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • [ ] Yes (Please add the Type: Breaking change label)
  • [ ] No

If Yes, what's the impact:

  • N/A

Pull request type

Please add the corresponding label for change this PR introduces:

  • Bugfix: Type: Bug
  • Feature/model/API additions: Type: Feature
  • Updates to docs or samples: Type: Documentation
  • Dependencies/code cleanup: Type: Maintenance

dbartol avatar May 01 '23 17:05 dbartol