-
Notifications
You must be signed in to change notification settings - Fork 132
Add encryption option to webhooks. #50
Comments
@NDevox A similar suggestion was offered by @bryanhelmig in PR #26 . I am not to sure he was explicitly advocating for a PR to add a persistable field to the AbstractHook model. With that said, I would definitely accept a contribution on documentation to roll out this solution or even some small utils to help a custom solution easier to implement out the box. |
So the implementation explained by @bryanhelmig is exactly the kind of thing I was thinking of implementing. I'm not sure I can see a way of doing it without adding a column however which is what he seems to ask for. The main issue I see is having the same shared secret between webhooks. Where do you store that? You could store it in the settings of the sender but you have to have ownership over both sender and receiver or be happy sharing your secret with everyone - in which case it isn't a secret. You could introduce a new model which links users to webhooks and holds the secret but this feels cumbersome. I feel like the best option is what I mentioned above where the field is added only if the user asks for it to be added I was initially thinking about making this non editable at first but to ensure a uniform shared secret between webhooks for a single user I think having it editable would be necessary. As mentioned I don't think this would be more than a couple lines added into the codebase giving a single line setup for a user to have hmac based auth. Would it be accepted as a PR? If not I'll do a custom setup and add documentation as a PR. |
Normally you'd reach for something like an environment variable here, just like any other API key or DB credential, etc. If you have distributed clients, you'd likely reach for a new DB table or something similar with: CREATE TABLE credentials (
secret varchar(32) NOT NULL,
user_id int(11) NOT NULL,
) And whatever you set up to consume your hooks (view/task/etc) could verify the secret matches the user. This is the same sort of dance you'd do with any other authorization setup. |
@bryanhelmig I meant on the server side 😉. In which case it would have to be a table I reckon. So my proposition for this would be:
Sound good or any feedback? |
Actually a better alternative to the Check if it exists first and if it doesn't generate it. That way if someone flips the With a |
Will someone implement it? |
This is unlikely to get prioritized by us @leiserfg -- if you fork and implement it we are happy to help you get it into mainline -- pull requests are welcome! |
Not sure if there is a better standard practice but using the webhooks I've noted that I need to setup an open POST hook endpoint on the server receiving the webhooks without any security. As a result anyone can send a webhook to me and I'd have to blindly accept it.
Another webhook system I've used would encrypt the request body using a shared secret created when setting up the webhook. That way no one could send a webhook unless they knew the shared secret.
I'd propose a method of adding a shared secret to this package - as an option in settings.
If the setting is present we add a new field to the
AbstractHook
and in the deliver_hook method we can encrypt the payload.This could be done as a customisation but I reckon it adds more value as part of this package - it should be a common use case as open ended hook endpoints are not great.
Happy to make the contribution unless it's not wanted/there is a better practice.
The text was updated successfully, but these errors were encountered: