Webhook Signature Verification #90

mjc-gh · 2017-08-26T19:07:06Z

Closes #83.

This to do for this PR:

Update the README
Get my specs running and add a few for this feature

coveralls · 2017-08-26T19:09:15Z

Coverage decreased (-3.0%) to 96.269% when pulling 546e454 on mikeycgto:webhook-signature-verification into 7e5ed1e on integrallis:master.

coveralls · 2017-08-26T19:09:15Z

Coverage decreased (-3.0%) to 96.269% when pulling 546e454 on mikeycgto:webhook-signature-verification into 7e5ed1e on integrallis:master.

coveralls · 2017-08-26T19:09:15Z

Coverage decreased (-3.0%) to 96.269% when pulling 546e454 on mikeycgto:webhook-signature-verification into 7e5ed1e on integrallis:master.

rmm5t · 2017-08-27T17:31:51Z

Add deprecation errors and/or warnings for removal of StripeEvent.authentication_secret

Let's keep things focused. Please don't remove or or change the behavior of the authentication_secret as part of this pull-request. Let's only consider deprecating it after we get signature verification fully working and released. The deprecation deserves to be part of a separate PR later.

mjc-gh · 2017-08-27T19:14:15Z

Sounds good. Had only included it in this PR cause the original issue mentioned that authentication_secret can be deprecated. I will remove that portion of the PR and keep both features in place.

Need to get the specs running locally so I can add some for this feature. Was having bundler version issues when I tried to run them initially.

coveralls · 2017-08-28T00:22:21Z

Coverage decreased (-0.7%) to 98.571% when pulling d2dbb83 on mikeycgto:webhook-signature-verification into 7e5ed1e on integrallis:master.

This configuration value is used to verify the cryptographic signature of a Stripe webhook. This commit also updates the webhook controller to verify the signature. See Stripe for more details: https://stripe.com/docs/webhooks#signatures

coveralls · 2017-08-28T00:34:55Z

Coverage decreased (-0.7%) to 98.571% when pulling 4b59c76 on mikeycgto:webhook-signature-verification into 7e5ed1e on integrallis:master.

rmm5t · 2017-08-28T17:49:56Z

Thanks for making the adjustments to this PR. Looks clean. Have you tried this branch against a live Stripe endpoint yet?

My only critique is that I'd prefer the tests to pass an actual HMAC signature header instead of mocking the call to Stripe::Webhook::Signature.verify_header (I prefer a classical style of testing where possible vs a mockist style), but there's no need for that to hold up this PR.

I can merge and release a new version later today if no one else objects.

Good work @mikeycgto!

mjc-gh · 2017-08-28T18:08:47Z

Thanks!

Yeah I do agree with you and prefer to avoid mocking when possible. My concern there was the specs would become to specific to actual the HMAC implementation in Stripe. Figured mocks would just be quicker to write and more future-proof if their verification process changes (assuming the API doesn't change 😃).

brandur · 2017-08-28T21:14:20Z

@rmm5t @mikeycgto Nice! I'd +1 trying to go a level deeper than the mock if possible. What you can do in this case is to call Stripe::Webhook::Signature.compute_signature(payload, secret) to generate a valid signature before validating it.

That way you can test more deeply than the mock and you don't have to rely on any particular crypto implementation — if we update to something beyond HMAC-SHA256, we'll be updating .compute_signature as well .verify_header within stripe-ruby.

mjc-gh · 2017-08-28T22:04:49Z

@brandur this was more involved than I thought it would be (have to include a timestamp and version scheme). I do agree it's better to use real signatures here though. Thanks for the feedback!

coveralls · 2017-08-28T22:18:02Z

Coverage decreased (-0.7%) to 98.571% when pulling 806c572 on mikeycgto:webhook-signature-verification into 7e5ed1e on integrallis:master.

brandur

Nice! Left a couple comments below.

(BTW, just so it's clear, I don't have any direct involvement in this project, and just took a look because a related thread had been bumped with this PR. I fully defer to the real maintainers on all things.)

brandur · 2017-08-28T22:16:47Z

spec/controllers/webhook_controller_spec.rb

+    after(:each) { StripeEvent.signing_secret = nil }
+
+    it "rejects invalid signatures" do
+      webhook_with_signature generate_signature('invalid payload'), id: 'evt_charge_succeeded'


Hm, I don't think I fully understand this test. Why does this one come out as invalid with the one below valid? It seems that it should be computing a valid signature for any value that you send to #generate_signature shouldn't it?

Perhaps I shouldn't use generate_signature? I wanted to have a spec that passes the correct format with the timestamp but with an incorrect signature. Maybe I should just pass a string directly here like so:

webhook_with_signature "t=#{timestamp},v1=invalid", id: 'evt_charge_succeeded'

Perhaps a better spec would be to "reject missing signature" where no header is provided? Any suggestions here @rmm5t?

I would suggest just passing in a "bogus" signature string and avoid the call to generate_signature.

brandur · 2017-08-28T22:17:26Z

spec/controllers/webhook_controller_spec.rb

+    end
+
+    let(:secret) { 'secret' }
+    let(:signature) { 'signature' }


I think this might be unused now.

Indeed 😄

brandur · 2017-08-28T22:18:00Z

spec/controllers/webhook_controller_spec.rb

+    end
+
+    def generate_signature(payload)
+      signature = Stripe::Webhook::Signature.send(:compute_signature, "#{timestamp}.#{payload}", secret)


Shoot. I forgot this is declared private. Sorry about that! I expect this to be pretty stable, but we might want to consider exposing this publicly for testing purposes if nothing else.

mjc-gh · 2017-08-29T02:07:17Z

Refactored the specs some to hopefully make more sense. Thanks again for the feedback!

coveralls · 2017-08-29T02:09:30Z

Coverage decreased (-0.7%) to 98.571% when pulling 8b53943 on mikeycgto:webhook-signature-verification into 7e5ed1e on integrallis:master.

coveralls · 2017-08-29T02:09:31Z

Coverage decreased (-0.7%) to 98.571% when pulling 8b53943 on mikeycgto:webhook-signature-verification into 7e5ed1e on integrallis:master.

coveralls · 2017-08-29T02:09:31Z

Coverage decreased (-0.7%) to 98.571% when pulling 8b53943 on mikeycgto:webhook-signature-verification into 7e5ed1e on integrallis:master.

coveralls · 2017-08-29T19:04:57Z

Coverage decreased (-0.7%) to 98.571% when pulling 01ffaa8 on mikeycgto:webhook-signature-verification into 6fdee50 on integrallis:master.

rmm5t · 2017-08-29T19:35:33Z

@mikeycgto Looks good to me. Are you done with your refactoring? I'm happy to merge now when ready.

mjc-gh · 2017-08-29T22:03:03Z

I'm good with merging. Thanks.

mjc-gh force-pushed the webhook-signature-verification branch from 546e454 to d2dbb83 Compare August 28, 2017 00:18

mjc-gh added 3 commits August 27, 2017 20:32

Add signing_secret for verifying webhooks

1c38eb8

This configuration value is used to verify the cryptographic signature of a Stripe webhook. This commit also updates the webhook controller to verify the signature. See Stripe for more details: https://stripe.com/docs/webhooks#signatures

Add specs for signing_secret feature

e5f34ec

Update README with signing_secret feature

4b59c76

mjc-gh force-pushed the webhook-signature-verification branch from d2dbb83 to 4b59c76 Compare August 28, 2017 00:33

rmm5t mentioned this pull request Aug 28, 2017

Support for Signed Web Hooks? #83

Closed

Use real signatures for secret_signing specs

806c572

brandur reviewed Aug 28, 2017

View reviewed changes

Refactor secret_signing specs

01ffaa8

mjc-gh force-pushed the webhook-signature-verification branch from 8b53943 to 01ffaa8 Compare August 29, 2017 18:50

rmm5t merged commit 313e531 into integrallis:master Aug 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webhook Signature Verification #90

Webhook Signature Verification #90

mjc-gh commented Aug 26, 2017 •

edited by rmm5t

coveralls commented Aug 26, 2017 •

edited

coveralls commented Aug 26, 2017

coveralls commented Aug 26, 2017

rmm5t commented Aug 27, 2017

mjc-gh commented Aug 27, 2017

coveralls commented Aug 28, 2017 •

edited

coveralls commented Aug 28, 2017 •

edited

rmm5t commented Aug 28, 2017

mjc-gh commented Aug 28, 2017

brandur commented Aug 28, 2017 •

edited

mjc-gh commented Aug 28, 2017

coveralls commented Aug 28, 2017 •

edited

brandur left a comment •

edited

brandur Aug 28, 2017

mjc-gh Aug 28, 2017

rmm5t Aug 29, 2017

brandur Aug 28, 2017

mjc-gh Aug 28, 2017

brandur Aug 28, 2017

mjc-gh commented Aug 29, 2017

coveralls commented Aug 29, 2017 •

edited

coveralls commented Aug 29, 2017

coveralls commented Aug 29, 2017

coveralls commented Aug 29, 2017 •

edited

rmm5t commented Aug 29, 2017

mjc-gh commented Aug 29, 2017

Webhook Signature Verification #90

Webhook Signature Verification #90

Conversation

mjc-gh commented Aug 26, 2017 • edited by rmm5t

coveralls commented Aug 26, 2017 • edited

coveralls commented Aug 26, 2017

coveralls commented Aug 26, 2017

rmm5t commented Aug 27, 2017

mjc-gh commented Aug 27, 2017

coveralls commented Aug 28, 2017 • edited

coveralls commented Aug 28, 2017 • edited

rmm5t commented Aug 28, 2017

mjc-gh commented Aug 28, 2017

brandur commented Aug 28, 2017 • edited

mjc-gh commented Aug 28, 2017

coveralls commented Aug 28, 2017 • edited

brandur left a comment • edited

Choose a reason for hiding this comment

brandur Aug 28, 2017

Choose a reason for hiding this comment

mjc-gh Aug 28, 2017

Choose a reason for hiding this comment

rmm5t Aug 29, 2017

Choose a reason for hiding this comment

brandur Aug 28, 2017

Choose a reason for hiding this comment

mjc-gh Aug 28, 2017

Choose a reason for hiding this comment

brandur Aug 28, 2017

Choose a reason for hiding this comment

mjc-gh commented Aug 29, 2017

coveralls commented Aug 29, 2017 • edited

coveralls commented Aug 29, 2017

coveralls commented Aug 29, 2017

coveralls commented Aug 29, 2017 • edited

rmm5t commented Aug 29, 2017

mjc-gh commented Aug 29, 2017

mjc-gh commented Aug 26, 2017 •

edited by rmm5t

coveralls commented Aug 26, 2017 •

edited

coveralls commented Aug 28, 2017 •

edited

coveralls commented Aug 28, 2017 •

edited

brandur commented Aug 28, 2017 •

edited

coveralls commented Aug 28, 2017 •

edited

brandur left a comment •

edited

coveralls commented Aug 29, 2017 •

edited

coveralls commented Aug 29, 2017 •

edited