search

openfoodfoundation / openfoodnetwork

Connect suppliers, distributors and consumers to trade local produce.
https://www.openfoodnetwork.org
GNU Affero General Public License v3.0
1.12k stars 723 forks source link

Add HMAC header for verification #10599

Open dacook opened 1 year ago

dacook commented 1 year ago

As suggested by Olivier:

How can you verify the order cycle open event is sent by the OFN instance (and not someone else) ? HMAC would be a nice and resilient feature (like github https://docs.github.com/en/webhooks-and-events/webhooks/securing-your-webhooks#validating-payloads-from-github) not too difficult to implement

https://openfoodnetwork.slack.com/archives/C026JMVK1DK/p1679388404086199?thread_ts=1679352493.562299&cid=C026JMVK1DK

dacook commented 1 year ago

I think we could use the user's API key as the secret key.

olivier5741 commented 1 year ago

I think we could use the user's API key as the secret key.

This could be a temporary workaround :). But it's interesting to have both separate because the way you can protect both is different and who is in charge is different.

The generation of the API key is OFN responsibility, the hash secret is the integrator responsibility (a bad hash secret will only affect the integrator).

For protection, on OFN side you only need a hash of the API key. Someone send you a request, you take the api key, you make a hash of it and compare it to the value of what you have in database. So if someone steels the database or manage to get access to the api key stored in there, they can't use them because they are hashed (ideal world ... their need to be some thinking on how to hash api keys) -> you don't need to ask to everybody to change its api key ... This can't be achieved for a webhook secret, you need the secret to create a hash.

Side note :) : for me, the api key should only be showed once since you would store a hash of it ... (I would consider it mandatory by law since it's an equivalent of a password ...)

RachL commented 1 year ago

@dacook is this a tech-debt issue?

dacook commented 1 year ago

Thanks Olivier, I definitely didn't think about it that much!

@RachL I don't consider it technically tech debt or a bug, but rather a security enhancement.

I guess if you have business-critical tasks relying on the webhook, and especially if you're performing actions based on a user's order details once paid, I can now see that this would be important. You wouldn't want somebody to fake the webhook to say that they paid for certain products that they didn't.

So I'd consider this a pre-requisite for any webhooks based on order data.