Accept array of secrets, not just one #777

nwf-msr · 2022-12-06T02:32:31Z

Resolves #770.

Behavior

Before the change?

Only one secret could be specified for a WebHook object.

After the change?

A bare secret or an array of secrets are each accepted. In the latter case, any match will be accepted.

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?

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.

And plumb through the option of taking an array of secrets

Add typechecking and integration tests Mention this ability in the README FIXES #770

gr2m

PR looks great so far! Can you please also update the ### Constructor options table?

Also can you run npm run lint:fix to make the prettier linter happy?

gr2m · 2022-12-06T17:33:06Z

can you look into the failing tests?

Please don't force-push changes, we don't mind a messy commit history in pull requests, we squash & merge them in the end anyway

There's doubtless a better way to do this, but Promise.any doesn't seem to be available.

nwf-msr · 2022-12-08T00:46:01Z

What'd I do to make CodeQL unhappy? :(

wolfy1339 · 2022-12-08T00:58:10Z

It's not you, it's CodeQL being uncooperative.

Just ignore it

gr2m

I think the pull request looks good as is.

Good point about the .sign() method. Given that the first secret in a given array of secrets has special meaning, I wonder if it would make more sense to keep the secret as is, but add an additional option such as alternativeSecrets or maybe some other name that better conveys that these secrets are used for incoming request verification only?

My understanding is that in your case, there will always only be one other secret. Do you see a use case where more than two secrets would be needed to be provided?

gr2m · 2022-12-14T01:40:34Z

README.md

+then update your Webhook receiver to accept only the new secret. Signatures will
+always be made with the first secret.


Suggested change

then update your Webhook receiver to accept only the new secret. Signatures will

always be made with the first secret.

then update your Webhook receiver to accept only the new secret. New signatures will

be calculated using the first secret.

gr2m · 2022-12-14T01:40:54Z

README.md

@@ -111,11 +123,14 @@ new Webhooks({ secret /*, transform */ });
        <code>
          secret
        </code>
-        <em>(String)</em>
+        <em>(String or String Array)</em>


Suggested change

<em>(String or String Array)</em>

<em>(String or Array of Strings)</em>

gr2m · 2022-12-14T01:42:37Z

README.md

+        Secret as configured in GitHub Settings.  If an array of strings, an
+        incoming webhook event will be accepted if its signature verifies
+        with any of the secrets given and signatures will always be generated
+        with the first secret.


Suggested change

Secret as configured in GitHub Settings. If an array of strings, an

incoming webhook event will be accepted if its signature verifies

with any of the secrets given and signatures will always be generated

with the first secret.

Secret as configured in GitHub Settings. If set to an array of strings, a

webhook request will be accepted if its signature verifies

with any of the given secrets. New signatures will always be

calculated using the first secret.

nwf-msr · 2022-12-16T15:57:17Z

I think the pull request looks good as is.

Good point about the .sign() method. Given that the first secret in a given array of secrets has special meaning, I wonder if it would make more sense to keep the secret as is, but add an additional option such as alternativeSecrets or maybe some other name that better conveys that these secrets are used for incoming request verification only?

That makes sense. I'll take a stab at that approach.

My understanding is that in your case, there will always only be one other secret. Do you see a use case where more than two secrets would be needed to be provided?

That's true and no, I don't see reason to need more than one active and one retiring secret, at least, at the moment. I'm tempted to leave alternativeSecrets as an array of strings, tho', just in case. It's not like it's hard or on the hot path most of the time.

gr2m · 2023-01-10T01:16:34Z

maybe call them alternativeSecretsForVerification to make clear they won't be used for the .sign() method?

nwf-msr · 2023-02-06T18:07:28Z

Belatedly, I've raised #811 .

wolfy1339 · 2023-02-10T01:48:18Z

Instead of opening a separate PR, you could have simply reused this one.

Closing in favour of #811

nwf-msr · 2023-02-10T01:50:33Z

I didn't reuse this one because I was advised not to force-push changes and enough time had elapsed that doing the work on a new checkout seemed wise.

wolfy1339 · 2023-02-10T01:54:57Z

When you need to update using a rebase, a force-push can be warranted.
When you simply always squash your changes, it can become harder to review changes since the last time it was reviewed, and we don't mind having many commits.

nwf-msr added 3 commits December 5, 2022 04:29

DRY out verify-and-recieve: appeal to our verify

a88e1a0

And plumb through the option of taking an array of secrets

Expose array of secrets option at toplevel

2dc6152

Add typechecking and integration tests Mention this ability in the README FIXES #770

gr2m added the Type: Feature New feature or request label Dec 6, 2022

gr2m requested changes Dec 6, 2022

View reviewed changes

nwf-msr force-pushed the 202212-770-secrets branch from b6e31fe to 2dc6152 Compare December 6, 2022 04:37

nwf-msr requested a review from gr2m December 6, 2022 04:39

nwf-msr and others added 4 commits December 7, 2022 19:26

When given multiple secrets, always sign with the first

ea8e99e

verify: serially loop over multiple secrets

5ac4f02

There's doubtless a better way to do this, but Promise.any doesn't seem to be available.

fix lint

0a5b6c6

Merge branch 'master' into 202212-770-secrets

304aeac

gr2m reviewed Dec 14, 2022

View reviewed changes

nwf-msr mentioned this pull request Feb 6, 2023

Ease key rotation via an array of fallback secrets #811

Closed

4 tasks

wolfy1339 closed this Feb 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept array of secrets, not just one #777

Accept array of secrets, not just one #777

nwf-msr commented Dec 6, 2022

gr2m left a comment •

edited

Loading

gr2m commented Dec 6, 2022

nwf-msr commented Dec 8, 2022

wolfy1339 commented Dec 8, 2022

gr2m left a comment

gr2m Dec 14, 2022

gr2m Dec 14, 2022

gr2m Dec 14, 2022

nwf-msr commented Dec 16, 2022

gr2m commented Jan 10, 2023

nwf-msr commented Feb 6, 2023

wolfy1339 commented Feb 10, 2023

nwf-msr commented Feb 10, 2023

wolfy1339 commented Feb 10, 2023

		then update your Webhook receiver to accept only the new secret. Signatures will
		always be made with the first secret.

	<em>(String or String Array)</em>
	<em>(String or Array of Strings)</em>

Accept array of secrets, not just one #777

Accept array of secrets, not just one #777

Conversation

nwf-msr commented Dec 6, 2022

Behavior

Before the change?

After the change?

Additional info

Pull request checklist

Does this introduce a breaking change?

Pull request type

gr2m left a comment • edited Loading

Choose a reason for hiding this comment

gr2m commented Dec 6, 2022

nwf-msr commented Dec 8, 2022

wolfy1339 commented Dec 8, 2022

gr2m left a comment

Choose a reason for hiding this comment

gr2m Dec 14, 2022

Choose a reason for hiding this comment

gr2m Dec 14, 2022

Choose a reason for hiding this comment

gr2m Dec 14, 2022

Choose a reason for hiding this comment

nwf-msr commented Dec 16, 2022

gr2m commented Jan 10, 2023

nwf-msr commented Feb 6, 2023

wolfy1339 commented Feb 10, 2023

nwf-msr commented Feb 10, 2023

wolfy1339 commented Feb 10, 2023

gr2m left a comment •

edited

Loading