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

[WIP] Add Postal support #166

Draft
wants to merge 10 commits into
base: master
from
Draft

[WIP] Add Postal support #166

wants to merge 10 commits into from

Conversation

@tiltec
Copy link

tiltec commented Sep 17, 2019

This is a very bare-bone implementation for the self-hosted Postal platform.

About Postal:

Postal is a complete and fully featured mail server for use by websites & web servers. Think Sendgrid, Mailgun or Postmark but open source and ready for you to run on your own servers.

We use this draft implementation since a few days to handle emails for our karrot.world web app. Before we used SparkPost via anymail. I had good experiences with anymail so far, that's why I'm keen on using it.
I'll improve this implementation during the next weeks as I find time. I'm happy about early feedback!

To do:

  • figure out how to do webhook authentication with Postal
  • support track_clicks and track_opens
  • add tests
  • add docs
tiltec added 2 commits Sep 17, 2019
@medmunds

This comment has been minimized.

Copy link
Contributor

medmunds commented Sep 19, 2019

This is great, thanks! I'm out of town right now, so just took a quick look at the code, but it looks good so far. Let me know if there are any specific questions I can answer.

As far as webhook auth, if Postal doesn't already have some means of signing webhook payloads, using Anymail's built-in http basic auth should be sufficient.

My memory is that aTech no longer offers a hosted Postal service, so we probably won't be able to include this in Anymail's live CI tests (unless you know someone running Postal who could set up an Anymail test account). But I suspect the Postal API isn't changing much, so regular live API testing might be less important than with other ESPs.

@medmunds medmunds added the new ESP label Sep 19, 2019
@tiltec

This comment has been minimized.

Copy link
Author

tiltec commented Sep 21, 2019

Thanks for having a look! Implementation is working fine so far, I'll see when I can get around to bring the PR into a mergable state.

As far as webhook auth, if Postal doesn't already have some means of signing webhook payloads, using Anymail's built-in http basic auth should be sufficient.

I wasn't able to get http basic auth working with Postal. I didn't investigate more, but it clearly didn't seem to be designed for this use case: the webhook URL validation doesn't allow the @ character.

My memory is that aTech no longer offers a hosted Postal service, so we probably won't be able to include this in Anymail's live CI tests (unless you know someone running Postal who could set up an Anymail test account). But I suspect the Postal API isn't changing much, so regular live API testing might be less important than with other ESPs.

As Postal is open-source, we could spin up our own testing instance in Travis (e.g. via docker image). But as you said, the API isn't changing much, so live tests seem safe to be left out for now.

@medmunds

This comment has been minimized.

Copy link
Contributor

medmunds commented Sep 24, 2019

I wasn't able to get http basic auth working with Postal. I didn't investigate more, but it clearly didn't seem to be designed for this use case: the webhook URL validation doesn't allow the @ character.

Ah. The nice thing about using HTTP basic auth for webhook authentication is that it tends to just work with software that hasn't put any special effort into webhook auth. Unless, you know, that software rolls its own regexp to (not quite accurately) validate webhook urls.

However, it sounds like Postal signs the webhook body, apparently using SHA-1 with /opt/postal/config/signing.key, and passes the base64-encoded signature in the X_POSTAL_SIGNATURE header. So I suppose you could verify that instead. (And include a docs warning box about SHA-1 security.)

@tiltec

This comment has been minimized.

Copy link
Author

tiltec commented Sep 24, 2019

Ah. The nice thing about using HTTP basic auth for webhook authentication is that it tends to just work with software that hasn't put any special effort into webhook auth. Unless, you know, that software rolls its own regexp to (not quite accurately) validate webhook urls.

Yeah, it's unfortunate. I did patch the regexp to allow basic auth, but it still didn't authenticate with anymail. I tend to not follow down this path for now, as I'm lacking Ruby skills.

However, it sounds like Postal signs the webhook body, apparently using SHA-1 with /opt/postal/config/signing.key, and passes the base64-encoded signature in the X_POSTAL_SIGNATURE header.

I'll do that. I managed to verify the signature with the cryptography library. So far, cryptography is not a dependency of anymail - do you think it's fine if I add it in setup.py as extras_require for Postal?

import base64
from cryptography.hazmat.primitives import serialization, hashes
from cryptography.hazmat.backends import default_backend
from cryptography.hazmat.primitives.asymmetric import padding

# read request.body and request.headers['X-Postal-Signature'] from request
# verify with public key (`p` part of Postal instance DKIM entry)

signature_decoded = base64.b64decode(signature)
pem_public_key = '-----BEGIN PUBLIC KEY-----\n' + dkim_p + '\n-----END PUBLIC KEY-----' 
public_key = serialization.load_pem_public_key(
  pem_public_key.encode(), 
  backend=default_backend()
) 
public_key.verify(
  signature_decoded, 
  body, 
  padding.PKCS1v15(), 
  hashes.SHA1()
)
# throws InvalidSignature if invalid
tiltec added 4 commits Sep 24, 2019
@medmunds

This comment has been minimized.

Copy link
Contributor

medmunds commented Sep 25, 2019

So far, cryptography is not a dependency of anymail - do you think it's fine if I add it in setup.py as extras_require for Postal?

Yes, perfect.

Copy link
Contributor

medmunds left a comment

This all looks great. Couple of minor comments on webhooks and setup extras.

anymail/webhooks/postal.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@medmunds

This comment has been minimized.

Copy link
Contributor

medmunds commented Sep 25, 2019

For adding cryptography, you'll also need to update tox.ini: extend the testenv config to install cryptography when the postal "factor" is being tested, and add the postal factor to the partial installation tests. And then add a line to .travis.yml to kick off a postal-only extras installation test.

anymail/webhooks/postal.py Outdated Show resolved Hide resolved
tiltec added 2 commits Oct 2, 2019
@tiltec

This comment has been minimized.

Copy link
Author

tiltec commented Oct 2, 2019

I probably can't spend much time on this in the next weeks, so I'd be interested in your preferred way forward. I'm happy to keep this as WIP PR, but I'd also spend some hours to get it into a mergeable state. What do you think?

@medmunds

This comment has been minimized.

Copy link
Contributor

medmunds commented Oct 2, 2019

I'm happy to add docs and tests, but it'll probably be a week or two before I'll be able to get to it. Let's keep the PR as WIP for now. I'll leave a comment before I start any work, just in case you get back to it before I do.

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.