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

Ease key rotation via an array of fallback secrets #811

Closed
wants to merge 1 commit into from
Closed

Ease key rotation via an array of fallback secrets #811

wants to merge 1 commit into from

Conversation

nwf-msr
Copy link

@nwf-msr nwf-msr commented 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 #777

Resolves #770.


Behavior

Before the change?

  • Only one secret could be specified for a WebHook object.

After the change?

  • Additional secrets are accepted for verification

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?

  • No

Pull request type

I do not seem to be able to add labels, but please add Type: Feature for me?


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 #777

FIXES #770
@nwf-msr nwf-msr mentioned this pull request Feb 6, 2023
4 tasks
@wolfy1339 wolfy1339 added the Type: Feature New feature or request label Feb 6, 2023
@wolfy1339
Copy link
Member

Would it be better to implement the behaviour in @octokit/webhooks-methods instead?

In the next major release in the works #792, there is no longer any custom verify methods in the codebase.

@nwf-msr
Copy link
Author

nwf-msr commented Feb 6, 2023

Could do if that's the better thing to do. Who else sits downstream of @octokit/webhooks-methods?

@wolfy1339
Copy link
Member

Could do if that's the better thing to do. Who else sits downstream of @octokit/webhooks-methods?

I don't think there are any of our packages that are downstream except for this one

@wolfy1339
Copy link
Member

Closing per author comment, #770 (comment)

@wolfy1339 wolfy1339 closed this Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[FEAT]: Facilitate webhook secret rotation
2 participants