Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEAT]: Facilitate webhook secret rotation #770

Open
1 task done
nwf-msr opened this issue Nov 8, 2022 · 5 comments
Open
1 task done

[FEAT]: Facilitate webhook secret rotation #770

nwf-msr opened this issue Nov 8, 2022 · 5 comments
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Feature New feature or request
Projects

Comments

@nwf-msr
Copy link

nwf-msr commented Nov 8, 2022

Describe the need

The current verifyAndReceive function assumes that there is exactly one secret in use at any time:

export async function verifyAndReceive(
state: State & { secret: string },

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

  • I agree to follow this project's Code of Conduct
@nwf-msr nwf-msr added Status: Triage This is being looked at and prioritized Type: Feature New feature or request labels Nov 8, 2022
@ghost ghost added this to Features in JS Nov 8, 2022
@gr2m
Copy link
Contributor

gr2m commented Nov 11, 2022

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.

@wolfy1339 wolfy1339 removed the Status: Triage This is being looked at and prioritized label Nov 11, 2022
@nwf-msr
Copy link
Author

nwf-msr commented Nov 11, 2022

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.

nwf-msr added a commit to nwf-msr/github-octokit-webhooks.js that referenced this issue Dec 6, 2022
Add typechecking and integration tests

FIXES octokit#770
nwf-msr added a commit to nwf-msr/github-octokit-webhooks.js that referenced this issue Dec 6, 2022
Add typechecking and integration tests
Mention this ability in the README

FIXES octokit#770
nwf-msr added a commit to nwf-msr/github-octokit-webhooks.js that referenced this issue Dec 6, 2022
Add typechecking and integration tests
Mention this ability in the README

FIXES octokit#770
nwf-msr added a commit to nwf-msr/github-octokit-webhooks.js that referenced this issue Feb 6, 2023
The core change is in src/verify.ts.  We don't really have to do
anything special here for constant-timedness.  Since each comparison is
constant time, the only timing differential is between all checks
(completely) failing and some check (completely) succeeding, and so the
only thing learned is whether a guess was completely right or wrong.

The rest of the changes here are plumbing this new mechanism up to the
top level and testing.

As suggested by @gr2m on octokit#777

FIXES octokit#770
nwf-msr added a commit to nwf-msr/github-octokit-webhooks.js that referenced this issue Feb 6, 2023
The core change is in src/verify.ts.  We don't really have to do
anything special here for constant-timedness.  Since each comparison is
constant time, the only timing differential is between all checks
(completely) failing and some check (completely) succeeding, and so the
only thing learned is whether a guess was completely right or wrong.

The rest of the changes here are plumbing this new mechanism up to the
top level and testing.

As suggested by @gr2m on octokit#777

FIXES octokit#770
@gr2m
Copy link
Contributor

gr2m commented Sep 18, 2023

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

@nwf-msr
Copy link
Author

nwf-msr commented Sep 18, 2023

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

@gr2m
Copy link
Contributor

gr2m commented Sep 18, 2023

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

@wolfy1339 wolfy1339 added the Status: Up for grabs Issues that are ready to be worked on by anyone label Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Feature New feature or request
Projects
No open projects
JS
  
Features
5 participants