Support hot-swapping of webhok secrets

[dbartol]

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

---

[JamieMagee]

@dbartol before you go too deep on this, would the IOptionsSnapshot pattern work?

[dbartol]

@JamieMagee Thanks for the pointer. This might fit better with IOptionsMonitor from that same link, since I'd expect the webhook processor to be a singleton. I'll give that a shot.

In any case, most of the work here was the refactoring, which should be helpful even without any hot-swapping support. If you'd prefer, I can take out the hot-swapping part of this PR and just leave the refactoring and multi-secret support while I try out IOptionsMonitor.

[JamieMagee]

Yeah, if it's not too much trouble, could you separate out the refactoring from the implementation with IOptionsMonitor?

[justinmchase]

I was thinking you would want an IGithubWebhookSecretResolver service instead of just a plain callback function but this is probably ok.

[JamieMagee]

@justinmchase I don't have permissions in this repository at the moment, but this PR isn't ready to be merged.

I'll followup with @dbartol, but I suspect he isn't going to pick this back up. In which case, I'll take on the refactor once I get my permissions back.

[justinmchase]

Sure, but in general I filed an issue a couple of days ago that this would fix, so one way or another the general idea of this PR looks pretty good.