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

Support multiple signing_secret #98

Closed
gustafr opened this issue Dec 7, 2017 · 11 comments
Closed

Support multiple signing_secret #98

gustafr opened this issue Dec 7, 2017 · 11 comments
Assignees

Comments

@gustafr
Copy link

gustafr commented Dec 7, 2017

Hi,
When using Stripe Connect, webhooks for the platform account and the connected accounts are handled separately, like this:

Endpoints receiving events from your account

and

Endpoints receiving events from Connect applications

What i noticed is that the signing_secret is then different for the two endpoints but in the stripe_event gem i can't figure out if it's possible to handle multiple signing_secrets like that?

@rmm5t
Copy link
Member

rmm5t commented Dec 7, 2017

Hi @gustafr. Thanks for chiming in about this. It's something I contemplated as a potential issue as I started the removal process of other authentication schemes (see #95, #96, #97); however, I wasn't sure about what the potential use-case for multiple signing_secret keys might be.

The plan was to move entirely to a single signing_secret requirement for v2.0.0, but that isn't going to work for your direct use-case.

I can think of a few options and/or workarounds.


  1. Workaround: Would it be possible to make your platform Account just one of your many other Connect accounts? For example, in my particular Stripe Connect application, the application eats its own dog food and is Connected just like any of my customer's accounts. I use just one Connect webhook endpoint and I get notifications about my customer's accounts and my own Account. You can sniff for your own internal webhooks by checking to see if event.account matches your platform Account ID.

  2. Option: I've considered the possibility of providing the ability to override the signing secret with an optional runtime signing_secret_retriever Proc (similar to the event_retriever or the new event_filter). By default, it would just return the signing_secret, but it could instead be used to choose a signing secret based on something in the webhook payload.

  3. Option: Another option would be to allow signing_secret to accept an array of keys. If there is more than one secret, all of the secrets could attempt verification for each webhook payload. As long as one successfully verifies, we can assume the payload to be good.

  4. Workaround: Stick with stripe_event v1.9.x for the time being.


Overall, I think I like Option 3 the best, because it is the simplest solution. Most applications should only need one signing_secret. There will be a few (like yours) that will need two. The question is whether there's a use case for N webhook subscriptions and N associated signing_secret keys for a single application.

I'm interested in other people's thoughts on this matter.

@gustafr
Copy link
Author

gustafr commented Dec 11, 2017

Hi @rmm5t,
Thank you for your reply! So about the need for N webhooks. I have not yet come across a use-case were more than two signing-secrets will be required. But I might be mistaken since I haven't used all of the Stripe-services yet. And of course this could change over time.
But anyone using Stripe Connect will be needing two signing-secrets, one for platform events and one for connected account events.
Personally I think option number 2 would be the neatest solution. Below I try to illustrate what is needed for the Stripe Connect usecase:

StripeEvent.platform_signing_secret
StripeEvent.connect_signing_secret

def verified_event
  payload   = request.body.read
  
  ## check to see if the payload includes an account parameter. If yes, this is a connect event, otherwise 
  it's a platform event.

  account = JSON.parse(payload)['account'].present?
  signature = request.headers['Stripe-Signature']
if account
  Stripe::Webhook.construct_event(payload, signature, StripeEvent.connect_signing_secret.to_s)
else
  Stripe::Webhook.construct_event(payload, signature, StripeEvent.platform_signing_secret.to_s)
end

This could be achived if we built a signing_secret_retriever Proc as you suggested. What do you think, any pitfalls? I haven't been giving this a whole lot of thought so I might just miss something important here. If i can be of any assistance in solving this, just let me know.
Thanks in advance.

@rmm5t
Copy link
Member

rmm5t commented Dec 11, 2017

But anyone using Stripe Connect will be needing two signing-secrets, one for platform events and one for connected account events.

This is not true, because I run a Stripe Connect application that only listens to the Connect webhook. If your Platform Account is registered as a Connected account for your Connect application, you can just subscribe to your single Connect webhook and get all the events through this same subscription (See Option 1 above). All you have to do is sniff for event.account and see if it matches your personal Stripe Account ID.

However, there might be admin-level events on the Account webhooks that are not delivered to the Connect endpoints (I'm not sure).


Personally I think option number 2 would be the neatest solution.

Option 2 (using a Proc to lookup a signing_secret) is probably the most robust solution, but it's also the hardest to document and it's the messiest from the perspective of developer adoption of this library (assuming that almost all use-cases need a maximum of 2 signing secrets). e.g. Most questions about this library in the past always seemed to be related to the optional event_retriever (though that was more of a necessity).

Below I try to illustrate what is needed for the Stripe Connect usecase:

I certainly do not want to distinguish config parameters like platform_signing_secret and connect_signing_secret. That's potentially more limiting (and confusing) than what we have in PR #97 now with just one signing_secret.


The more I noodle on this problem, the more I lean towards Option 3 and allowing for an optional array of signing secrets.

# For most Stripe applications:
StripeEvent.signing_secret = ENV["STRIPE_SIGNING_SECRET"]

# For some Stripe Connect applications:
StripeEvent.signing_secrets = [
  ENV["STRIPE_ACCOUNT_SIGNING_SECRET"],
  ENV["STRIPE_CONNECT_SIGNING_SECRET"],
]

Super easy for developer adoption, and relatively easy to implement in this library. There is almost zero downside, aside from the potential of an extra verification check per webhook event when multiple signing secrets are configured (but let the robots do our work for us, and the alternative of a lookup proc is likely just as much processing).


There's another option I can think of (let's call this Option 5), but it's the most complex, and it might only yield diminishing returns. That option would be to extend the config settings to the namespacing of the library itself and allow for multiple namespaces at once. In other words, you'd have entirely different subscribers for each of your different webhooks. This would likely require a different callback URL for each webhook as well, but you could configure a different signing_secret per callback endpoint. As I write this, I realize that I really don't like this option, but I wanted to throw it out there while brainstorming. This is likely an over-abstraction of the underlying problem.

@rmm5t
Copy link
Member

rmm5t commented Dec 11, 2017

Just a status update here. This issue is going to block a v2.0.0 release. I.e. We're going to address a solution before v2.0.0 goes out. Likely solved via Option 3.

@rmm5t
Copy link
Member

rmm5t commented Dec 11, 2017

@gustafr et al,

Please review #99. Most relevant to the discussion here is the section added to the README for how support for multiple signing secrets is enabled:


Support for multiple signing secrets

Sometimes, you'll have multiple Stripe webhook subscriptions pointing at your application each with a different signing secret. For example, you might have both a main Account webhook and a webhook for a Connect application point at the same endpoint. It's possible to configure an array of signing secrets using the signing_secrets configuration option. The first one that successfully matches for each incoming webhook will be used to verify your incoming events.

StripeEvent.signing_secrets = [
  Rails.application.secrets.stripe_account_signing_secret,
  Rails.application.secrets.stripe_connect_signing_secret,
]

(NOTE: signing_secret= and signing_secrets= are just aliases for one another)

@gustafr
Copy link
Author

gustafr commented Dec 12, 2017

Thank you @rmm5t for addressing this issue so swiftly, I really appreciate it! In my particular usecase this solution will mean quite a few extra verification checks but as you mention, maybe the Proc-solution wouldn't be more effective. I have reviewed #99 and I think the readme update is crystal-clear. Great work!

@rmm5t
Copy link
Member

rmm5t commented Dec 12, 2017

Thank you @rmm5t for addressing this issue so swiftly, I really appreciate it!

You are very welcome. I think this was a great addition to the library.

In my particular usecase this solution will mean quite a few extra verification checks but as you mention, maybe the Proc-solution wouldn't be more effective.

  1. Keep in mind the v1.x solution required a blocking network lookup of each event against the Stripe API. An extra HMAC digest here and there with the v2.x solution (when multiple secrets are configured) isn't much. We're talking hundreds of milliseconds in v1 vs nanoseconds in v2 (i.e. webhook event verification should be 2 or 3 orders of magnitude faster now).
  2. With that in mind, I did just further optimize the implementation to perform the absolute minimum number of verification header checks when multiple secrets are configured. See 6997993.

I have reviewed #99 and I think the readme update is crystal-clear. Great work!

Thanks! 🍻

@rmm5t rmm5t closed this as completed in #99 Dec 12, 2017
@rmm5t
Copy link
Member

rmm5t commented Dec 12, 2017

@gustafr I just merged everything into the master branch.

Could you please test your use-cases for multiple signing secrets against the master branch? This will help me release v2.0.0 sooner.

gem "stripe_event", github: "integrallis/stripe_event"

Otherwise, I'll plan to release an official v2.0.0 within the next few days.

@gustafr
Copy link
Author

gustafr commented Dec 13, 2017

Great news @rmm5t! I will test my use-cases against the master branch tomorrow and get back to you as soon as I'm done.

@gustafr
Copy link
Author

gustafr commented Dec 14, 2017

@rmm5t the multiple signing secrets are working as expected. So from my point of view all should be fine.

@rmm5t
Copy link
Member

rmm5t commented Dec 14, 2017

Thanks @gustafr. v2.0.0 is out in the wild 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants