search

octokit / webhooks.js

GitHub webhook events toolset for Node.js
MIT License
308 stars 80 forks source link

[FEAT]: Facilitate webhook secret rotation #770

Open nwf-msr opened 1 year ago

nwf-msr commented 1 year ago

Describe the need

The current verifyAndReceive function assumes that there is exactly one secret in use at any time: https://github.com/octokit/webhooks.js/blob/d6286fa324b65716c12ce03efe338faf0e9d2338/src/verify-and-receive.ts#L10-L11 which frustrates secret rotation, which can be a requirement in organizational settings. In particular, the current design essentially forces either programmer suffering (maintaining multiple webhook objects) or a window in which the sender and receiver will disagree about the (singular) secret value.

It would be ideal if, instead, verifyAndReceive accepted a list of secrets and attempted verification against each one. Then key rotation is straightforward:

  1. Generate a new secret.
  2. Update the receiver to accept messages MAC'd by this secret. Wait for things to settle.
  3. Update GitHub to send messages with this secret. Wait for things to settle.
  4. Remove the old secret from the receiver.

SDK Version

No response

API Version

No response

Relevant log output

No response

Code of Conduct

gr2m commented 1 year ago

I think your use case is valid, we could support setting the secret option to an array of strings.

But I would not permit it to be mutable. That would be a different feature, and if we were to decide to support it, I'd make it explicit with a callback function instead of a mutable array.

nwf-msr commented 1 year ago

Oh, yes, sorry, I hadn't meant to suggest a dynamic list of secrets. While I can see it being useful... selfishly, my use case has the WebHook listeners as single-shot Azure Function Apps, so it should suffice to change the secrets registered with those to ensure that all subsequent calls are checked against updated values.

gr2m commented 12 months ago

there is now a verifyWithFallback method that we could expose on the webhooks instance: https://github.com/octokit/webhooks-methods.js#verifywithfallback

nwf-msr commented 12 months ago

Yes, sorry; that was always supposed to be the next step after https://github.com/octokit/webhooks-methods.js/pull/134 landed, but I have been transferred internally and my prior projects are being held on minimal life support.

gr2m commented 12 months ago

no worries at all, I just wanted to add the context so folks know about it