Support hot-swapping of webhok secrets #262
Closed
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #261
Resolves #24
(In draft until I finish implementing tests)
Behavior
Before the change?
After the change?
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 existingWebhookEventProcessor
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 newValidateAndProcessWebhookAsync()
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 forProcessWebhookAsync()
), the bodyStream
, 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 astring
, 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 aWebHookValidationError
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
Does this introduce a breaking change?
Please see our docs on breaking changes to help!
Type: Breaking change
label)If
Yes
, what's the impact:Pull request type
Please add the corresponding label for change this PR introduces:
Type: Bug
Type: Feature
Type: Documentation
Type: Maintenance