Skip to content
This repository has been archived by the owner on May 5, 2021. It is now read-only.

Add encryption option to webhooks. #50

Open
NDevox opened this issue Jan 23, 2018 · 7 comments
Open

Add encryption option to webhooks. #50

NDevox opened this issue Jan 23, 2018 · 7 comments

Comments

@NDevox
Copy link

NDevox commented Jan 23, 2018

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.

@avelis
Copy link
Contributor

avelis commented Jan 23, 2018

@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.

@NDevox
Copy link
Author

NDevox commented Jan 23, 2018

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.

@bryanhelmig
Copy link
Member

The main issue I see is having the same shared secret between webhooks. Where do you store that?

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.

@NDevox
Copy link
Author

NDevox commented Jan 24, 2018

@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:

  1. An AbstractSecret class which maps OneToOne user to secret. with a Secret class which is to AbstractSecret what Hook is to AbstractHook.
  2. The same as above, a config value in settings which defines whether we want to use the secret or not.
  3. If we want to use secret we need a method of then generating a secret for the user and displaying it. The simplest method is probably a post_save signal for hooks which check if a user has a secret, if not generate one. And then display in the admin via a property on AbstractHook.

Sound good or any feedback?

@NDevox
Copy link
Author

NDevox commented Jan 24, 2018

Actually a better alternative to the post_save signal would be to generate the secret in the property.

Check if it exists first and if it doesn't generate it. That way if someone flips the REST_HOOK_ENCRYPT setting (or whatever it will be called) after creating Hooks the secret will still be generated automatically.

With a post_save flipping the setting later would break the code.

@leiserfg
Copy link

Will someone implement it?

@bryanhelmig
Copy link
Member

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!

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

No branches or pull requests

4 participants